Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions

On Tue, Jun 02, 2026 at 03:35:44AM +0300, Andrey Chernyy wrote:
> While looking at pgxml_xpath(), I noticed one more nearby cleanup gap.
> pgxml_xpath() builds an xpath_workspace before returning it to callers.
> The callers already wrap pgxml_xpath() with PG_TRY/PG_CATCH, but if an
> ERROR is thrown before pgxml_xpath() returns, the assignment of the
> workspace pointer in the caller has not completed yet. The caller-side
> PG_CATCH blocks therefore cannot call cleanup_workspace() for the
> partially-built workspace.

Aha, nice. That's because xmlXPathNewContext() can fail internally on
a call of xmlMalloc().

One thing that slightly confused me is that it could be possible to do
twice cleanup_workspace() for the callers of pgxml_xpath(), but that's
fine: once if the TRY/CATCH block of pgxml_xpath() fails and a second
time in the TRY/CATCH block of xpath_string(), xpath_number() or
xpath_bool().

Now wait a minute, something is off in upstream with
xmlXPathCompiledEval(), no? This stuff does a chain of
xmlXPathCompiledEval() -> xmlXPathCompiledEvalInternal() ->
xmlXPathCompParserContext(). xmlXPathCompParserContext() may fail on
a xmlMalloc(), and xmlXPathCompiledEvalInternal() has the idea to
return the same result if given NULL in input by its caller or on OOM.
That means that we could incorrectly assign a NULL result that should
not be. I don't think that there is much we can do in the Postgres
code because we have no idea of the error state (aka valid NULL or
just an OOM), but that may be worth mentioning to upstream, where they
would need a new "extensible" API with an error reason or a different
error code depending on the failure. As far as I can see we are doing
nothing wrong in xml2. Well, at least nothing worse than the current
deal. :)

The set of v2-0001~0004 merged together should be fine as final
solution, after double-checking all the callers with the set applied.
--
Michael