Skip to content

ext/xsl: handle xsltNewTransformContext allocation failure#21887

Open
iliaal wants to merge 2 commits intophp:PHP-8.4from
iliaal:fix/xsl-transformcontext-null-8.4
Open

ext/xsl: handle xsltNewTransformContext allocation failure#21887
iliaal wants to merge 2 commits intophp:PHP-8.4from
iliaal:fix/xsl-transformcontext-null-8.4

Conversation

@iliaal
Copy link
Copy Markdown
Contributor

@iliaal iliaal commented Apr 27, 2026

Two related defensive fixes in ext/xsl, folded per the #21888 review.

  • xsltNewTransformContext returns NULL on libxslt-internal allocation failure; the next line dereferenced ctxt to set _private and segfaulted. Bail through the existing out: label, which handles secPrefs and intern->doc cleanup. xsltFreeTransformContext(NULL) is a documented no-op in libxslt.
  • If xmlStrdup fails for either href or prefix in xsl_add_ns_def, the partially-constructed xmlNs (NULL href, or NULL prefix when one was expected) was linked into node->nsDef. Subsequent libxml2 traversal of the namespace chain dereferenced those NULLs. Free the xmlNs via xmlFreeNs and return without linking it.

xsltNewTransformContext can return NULL on libxslt-internal allocation
failure. The next line dereferences ctxt unconditionally to set
ctxt->_private, segfaulting on OOM. Bail through the existing out:
label, which already handles secPrefs/intern->doc cleanup.

xsltFreeTransformContext(NULL) is a documented no-op in libxslt.
@devnexen
Copy link
Copy Markdown
Member

I see .... can you do just one PR please for both. Those are kind of unlikely to happen, can t reproduce beside OOM ... Thanks.

@iliaal
Copy link
Copy Markdown
Contributor Author

iliaal commented Apr 27, 2026

Can do, I got a little gun shy with combining things after previous feedback 🤣 Can I combine all 3 (#21886, #21887 and #21888)?

@devnexen
Copy link
Copy Markdown
Member

I do not mind keeping 21886 separated however the two others are a bit more like "defensive programming".

@iliaal
Copy link
Copy Markdown
Contributor Author

iliaal commented Apr 27, 2026

Ok, I'll keep 86 separate, fold 88 into this one and close 88.

If xmlStrdup fails for either href or prefix in xsl_add_ns_def, the
malformed xmlNs (NULL href, or NULL prefix when one was expected) was
linked into node->nsDef. Subsequent libxml2 traversal of the namespace
chain dereferenced those NULLs.

Free the xmlNs via xmlFreeNs and return without linking it.
@ndossche
Copy link
Copy Markdown
Member

Subsequent libxml2 traversal of the namespace chain dereferenced those NULLs.

Is that true or is that what your agent says? The prefix can certainly be NULL, and I believe the href can also be NULL.
It might not make much sense to try to recover in those cases because when the memory is exhausted the C14N code from libxml will also fail. All that matters then is making sure the interpreter doesn't hard crash.

@iliaal
Copy link
Copy Markdown
Contributor Author

iliaal commented Apr 27, 2026

  1. xsltNewTransformContext can return null on OOM, so null is strictly possible, although not something you could trigger via PHP code, and one could argue once you hit OOM everything is kinda buggered anyway, but a safety check doesn't hurt.

  2. The 2nd bit (from ext/xsl: discard partially-constructed xmlNs on xmlStrdup failure #21888) now folded here, is kinda it looks right but in reality not so much. NULL href is tolerated by libxml2/libxslt traversal (c14n null-coalesces, namespaces.c null-checks, xmlStr* are NULL-safe), so you are right there is no NULL deref here, the cleanup of xmlFreeNs(ns) would only happen happen on OOM, which again kind pointless since you are almost out of memory anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants