Free input provider callbacks on cleanup#167
Open
shunkica wants to merge 6 commits into
Open
Conversation
XmlDocument.fromString/fromBuffer rejected well-formed documents whenever libxml2 emitted any diagnostic, including warning-level ones, and leaked the parsed C document on that throw. Two defects in parse() (src/document.mts): 1. Failure was decided by presence of any diagnostic (errDetails.length > 0), never by severity, so warnings (XML_ERR_WARNING = 1) were treated as fatal. Now failure is gated on level >= XML_ERR_ERROR (2), so warnings pass and error/fatal diagnostics (incl. XML_PARSE_DTDVALID validity errors) still throw. 2. The pre-throw cleanup guard was inverted (`if (!xml) xmlFreeDoc(xml)`): it freed only when the pointer was null and skipped freeing a real document, which then leaked since the throw precedes wrapper/finalizer creation. The guard is corrected to free the produced document before throwing. To make surviving warnings observable (they were silently discarded), capture the diagnostic severity and expose non-fatal diagnostics on the parsed document: - XmlErrorStruct.level (xmlError offset 12) + ErrorDetail.level - XmlDocument.warnings: ErrorDetail[] (level-1 diagnostics; [] for clean parses) Existing details assertions updated for the new level field; adds tests for warning acceptance, warning surfacing, validity-error rejection, and no-leak.
match() used a prototype-chain lookup (this._data[filename] != null), so a document referencing a name like 'toString' matched Object.prototype members. libxml2 then called open() with a non-buffer value, and read() later threw 'TypeError: data.slice is not a function' instead of failing gracefully. Use an own-property check plus Uint8Array validation via a shared buffer() helper; open() now returns number | undefined (matching XmlInputProvider and the fs provider's no-match signal). Add es2022.object lib so Object.hasOwn type-checks (emit target stays es2021).
) xmlXPathCompiledEval returns NULL on a runtime evaluation failure (undefined namespace prefix, variable, or function). compiledXPathEval returned it unchecked, so get()/eval() dereferenced the null pointer and threw the generic XmlError: 'Access with null pointer' — indistinguishable from an internal bug. Reset the last error before evaluation and, when the result is NULL, throw XmlXPathError forwarding libxml2's own diagnostic, e.g.: Failed to evaluate XPath expression '//x:y': Undefined namespace prefix: x Uses the already-exported xmlGetLastError/xmlResetLastError; no build change.
…chars Closes jameslan#164. xmlSaveToIO/xmlOutputBufferCreateIO leaked an outputHandlerStorage slot whenever libxml2 returned NULL (e.g. an unsupported encoding), because the close callback that frees the slot never fired. XmlDocument.save/ XmlElement.save also silently produced empty output in that case. The allocate/free-on-null lifecycle is now centralized in a withOutputHandler helper, and xmlSaveToIO throws XmlError naming the unsupported encoding instead of returning NULL. XmlStringOutputBufferHandler decoded each write() as a standalone buffer, corrupting multibyte characters split across chunks. It now decodes with { stream: true } and flushes in close().
xmlRegisterInputProvider adds four wasm function-table entries per call, one per callback closure, and xmlCleanupInputProvider reclaimed none of them. addFunction cannot reuse an entry until removeFunction releases it, so every register grew the table by four permanently and pinned the provider and its buffers for the process lifetime. Export Emscripten's removeFunction, track the entries of each registration and release them in cleanup. A registration that libxml2 refuses now frees its entries as well, instead of stranding them.
|
Coverage after merging fix/input-provider-table-leak into master will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #166
Fix
xmlRegisterInputProvideradds four wasm function-table entries per call, one percallback closure (
match/open/read/close), andxmlCleanupInputProviderreclaimed none of them.
addFunctioncannot reuse an entry untilremoveFunctionreleases it, so every register grew the table by four permanently and pinned the
provider, and any buffers it captured, for the process lifetime.
binding/exported-runtime-functions.txtexportsremoveFunction(
-s ALLOW_TABLE_GROWTH=1is already set, so no other emcc change is needed).src/libxml2.mtstracks the entries of each registration and releases them incleanup. libxml2 keeps owning the callback stack and its precedence, and the
provider's file descriptors still pass straight through.