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

On Mon, Jun 01, 2026 at 01:01:24AM +0300, Andrey Chernyy wrote:

This is related to, but not fixed by, the recent xml2/libxml
error-handling work from BUG #18943 / commit 732061150b0. That patch
improved cleanup and OOM handling around libxml calls, while these
cases are successful execution path leaks.

These are long-standing leaks and look like candidates for
back-patching to all supported branches.

Right. I have missed that xmlXPathCastNodeToString() does an
allocation of the result, and it is documented in the upstream code
that the caller is responsible for freeing what's been returned.

+   xmlXPathContextPtr volatile ctxt = NULL;
+   xmlXPathObjectPtr volatile res = NULL;
+   xmlXPathCompExprPtr volatile comppath = NULL;
+   xmlChar    *volatile resstr = NULL;

For these fout, ctxt is reachable once xmlXPathNewContext() does not
return NULL. For res, that's xmlXPathCompiledEval, and caller has to
free the result (documented). comppath is limited to the case where
the call of xmlXPathCtxtCompile() allocates a result, returns non-NULL
and has an error, which is a short window, still reachable. resstr
can be reached under when pg_xml_error_occurred() fails with an
allocation done.

I can see two more problematic patterns, that happen in error-only
paths still are worth addressing:
- pgxmlNodeSetToText() is missing a xmlFree() for "result".
- xmlXPathCtxtCompile() in pgxml_xpath(), where pg_xml_error_occurred
is reached and an allocation is done.

732061150b0 was only done on HEAD because these leaks were considered
as not worth the trouble in the back branches. This could qualify as
an open item, even if the leaks you are pointing at here are much
older than that.

Attached is a patch for the code paths I have grabbed on the way.
I'll take care of that, merging everything together. Thanks for the
report!
--
Michael

Hi Michael,

I checked v2, and the two additional error-path cases make sense to me.

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.

Attached is an incremental patch on top of your v2-0001..v2-0003. It
adds local cleanup in pgxml_xpath(), tracks comppath for the error path,
and checks xmlXPathNewContext() before dereferencing the returned
context. I kept it separate for review, but it can of course be folded
if that makes the final series cleaner.

I also used the attached manual repro script, which catches repeated
XPath syntax errors in a single backend and samples VmRSS from /proc.
On my checkout without this pgxml_xpath() cleanup it kept growing after
each round, for example:

postgres=# \i xml2-pgxml-xpath-error-leak-repro.sql
NOTICE: error i=1, failures=20, total_kb=109748, diff_kb=88008
NOTICE: error i=2, failures=20, total_kb=193016, diff_kb=83268
NOTICE: error i=3, failures=20, total_kb=276220, diff_kb=83204
NOTICE: error i=4, failures=20, total_kb=359428, diff_kb=83208
NOTICE: error i=5, failures=20, total_kb=442632, diff_kb=83204
NOTICE: error i=6, failures=20, total_kb=525840, diff_kb=83208
NOTICE: error i=7, failures=20, total_kb=609044, diff_kb=83204
NOTICE: error i=8, failures=20, total_kb=692252, diff_kb=83208
NOTICE: error i=9, failures=20, total_kb=775456, diff_kb=83204
NOTICE: error i=10, failures=20, total_kb=858664, diff_kb=83208

With this patch applied, it plateaued after the initial warmup:

postgres=# \i xml2-pgxml-xpath-error-leak-repro.sql
NOTICE: error i=1, failures=20, total_kb=22672, diff_kb=972
NOTICE: error i=2, failures=20, total_kb=22692, diff_kb=20
NOTICE: error i=3, failures=20, total_kb=22704, diff_kb=8
NOTICE: error i=4, failures=20, total_kb=22700, diff_kb=-8
NOTICE: error i=5, failures=20, total_kb=22704, diff_kb=4
NOTICE: error i=6, failures=20, total_kb=22704, diff_kb=0
NOTICE: error i=7, failures=20, total_kb=22708, diff_kb=4
NOTICE: error i=8, failures=20, total_kb=22704, diff_kb=-8
NOTICE: error i=9, failures=20, total_kb=22708, diff_kb=4
NOTICE: error i=10, failures=20, total_kb=22708, diff_kb=-4

--
Andrey Chernyy

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

Thanks, Michael.

Agreed about xmlXPathCompiledEval(); without a reliable error indication
from libxml2, xml2 cannot distinguish a valid NULL result from an
internal failure locally.

The v2-0001..v2-0004 merge sounds good to me.

--
Andrey Chernyy