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
From 7f0302c0e4a74b3ec3f497c00a7f853c1e0fbbd3 Mon Sep 17 00:00:00 2001 From: Andrey Chernyy <[email protected]> Date: Mon, 25 May 2026 22:12:02 +0300 Subject: [PATCH v2 1/3] Fix libxml string leak in contrib/xml2 xpath_list xmlXPathCastNodeToString() returns a libxml-allocated xmlChar *, but pgxmlNodeSetToText() passed it directly to xmlBufferWriteCHAR() in the plain separator path. Since xmlBufferWriteCHAR() copies the string rather than taking ownership, successful xpath_list() calls leaked one string per emitted node. Store the cast result locally and free it with xmlFree() after writing it to the buffer. --- contrib/xml2/xpath.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c index 7bf477e0c3f2..94819961787e 100644 --- a/contrib/xml2/xpath.c +++ b/contrib/xml2/xpath.c @@ -147,6 +147,7 @@ pgxmlNodeSetToText(xmlNodeSetPtr nodeset, { volatile xmlBufferPtr buf = NULL; xmlChar *volatile result = NULL; + xmlChar *volatile str = NULL; PgXmlErrorContext *xmlerrcxt; /* spin up some error handling */ @@ -172,8 +173,14 @@ pgxmlNodeSetToText(xmlNodeSetPtr nodeset, { if (plainsep != NULL) { - xmlBufferWriteCHAR(buf, - xmlXPathCastNodeToString(nodeset->nodeTab[i])); + str = xmlXPathCastNodeToString(nodeset->nodeTab[i]); + if (str == NULL || pg_xml_error_occurred(xmlerrcxt)) + xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY, + "could not allocate node text"); + + xmlBufferWriteCHAR(buf, str); + xmlFree(str); + str = NULL; /* If this isn't the last entry, write the plain sep. */ if (i < (nodeset->nodeNr) - 1) @@ -216,6 +223,8 @@ pgxmlNodeSetToText(xmlNodeSetPtr nodeset, } PG_CATCH(); { + if (str) + xmlFree(str); if (buf) xmlBufferFree(buf); -- 2.54.0
From e539f2f11bc892dd299ef13080e6b338202fdf9e Mon Sep 17 00:00:00 2001 From: Andrey Chernyy <[email protected]> Date: Tue, 26 May 2026 00:11:06 +0300 Subject: [PATCH v2 2/3] Fix libxml leaks in contrib/xml2 xpath_table xpath_table() did not release libxml objects allocated while evaluating XPath expressions. xmlXPathCompiledEval() returns an xmlXPathObjectPtr that must be freed, and string results copied into the tuple input array must be freed after BuildTupleFromCStrings() has consumed them. Track the current libxml objects across the existing PG_TRY block so they are also released on error. --- contrib/xml2/xpath.c | 47 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c index 94819961787e..ac140a640e07 100644 --- a/contrib/xml2/xpath.c +++ b/contrib/xml2/xpath.c @@ -643,6 +643,10 @@ xpath_table(PG_FUNCTION_ARGS) StringInfoData query_buf; PgXmlErrorContext *xmlerrcxt; volatile xmlDocPtr doctree = NULL; + xmlXPathContextPtr volatile ctxt = NULL; + xmlXPathObjectPtr volatile res = NULL; + xmlXPathCompExprPtr volatile comppath = NULL; + xmlChar *volatile resstr = NULL; InitMaterializedSRF(fcinfo, MAT_SRF_USE_EXPECTED_DESC); @@ -662,7 +666,7 @@ xpath_table(PG_FUNCTION_ARGS) attinmeta = TupleDescGetAttInMetadata(rsinfo->setDesc); - values = (char **) palloc(rsinfo->setDesc->natts * sizeof(char *)); + values = (char **) palloc0(rsinfo->setDesc->natts * sizeof(char *)); xpaths = (xmlChar **) palloc(rsinfo->setDesc->natts * sizeof(xmlChar *)); /* @@ -732,10 +736,6 @@ xpath_table(PG_FUNCTION_ARGS) { char *pkey; char *xmldoc; - xmlXPathContextPtr ctxt; - xmlXPathObjectPtr res; - xmlChar *resstr; - xmlXPathCompExprPtr comppath; HeapTuple ret_tuple; /* Extract the row data as C Strings */ @@ -780,6 +780,11 @@ xpath_table(PG_FUNCTION_ARGS) had_values = false; for (j = 0; j < numpaths; j++) { + ctxt = NULL; + res = NULL; + comppath = NULL; + resstr = NULL; + ctxt = xmlXPathNewContext(doctree); if (ctxt == NULL || pg_xml_error_occurred(xmlerrcxt)) xml_ereport(xmlerrcxt, @@ -798,6 +803,7 @@ xpath_table(PG_FUNCTION_ARGS) /* Now evaluate the path expression. */ res = xmlXPathCompiledEval(comppath, ctxt); xmlXPathFreeCompExpr(comppath); + comppath = NULL; if (res != NULL) { @@ -842,8 +848,16 @@ xpath_table(PG_FUNCTION_ARGS) * result tuple. */ values[j + 1] = (char *) resstr; + resstr = NULL; + } + + if (res != NULL) + { + xmlXPathFreeObject(res); + res = NULL; } xmlXPathFreeContext(ctxt); + ctxt = NULL; } /* Now add the tuple to the output, if there is one. */ @@ -854,6 +868,16 @@ xpath_table(PG_FUNCTION_ARGS) heap_freetuple(ret_tuple); } + /* BuildTupleFromCStrings() has copied the values. */ + for (j = 1; j < rsinfo->setDesc->natts; j++) + { + if (values[j] != NULL) + { + xmlFree((xmlChar *) values[j]); + values[j] = NULL; + } + } + rownr++; } while (had_values); } @@ -870,6 +894,19 @@ xpath_table(PG_FUNCTION_ARGS) } PG_CATCH(); { + if (resstr != NULL) + xmlFree(resstr); + for (j = 1; j < rsinfo->setDesc->natts; j++) + { + if (values[j] != NULL) + xmlFree((xmlChar *) values[j]); + } + if (res != NULL) + xmlXPathFreeObject(res); + if (comppath != NULL) + xmlXPathFreeCompExpr(comppath); + if (ctxt != NULL) + xmlXPathFreeContext(ctxt); if (doctree != NULL) xmlFreeDoc(doctree); -- 2.54.0
From 6ff5a77a921eb3fdaa25e4e7884d7c1ab17a63e3 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 1 Jun 2026 12:59:05 +0900 Subject: [PATCH v2 3/3] xml2: Fix two more leaks - In pgxmlNodeSetToText(), the result could be leaked on error. - In pgxml_xpath(), a context could be compiled and would leak if the - call fails. --- contrib/xml2/xpath.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c index ac140a640e07..9fe75cb5ff40 100644 --- a/contrib/xml2/xpath.c +++ b/contrib/xml2/xpath.c @@ -223,6 +223,8 @@ pgxmlNodeSetToText(xmlNodeSetPtr nodeset, } PG_CATCH(); { + if (result) + xmlFree(result); if (str) xmlFree(str); if (buf) @@ -513,8 +515,12 @@ pgxml_xpath(text *document, xmlChar *xpath, PgXmlErrorContext *xmlerrcxt) /* compile the path */ comppath = xmlXPathCtxtCompile(workspace->ctxt, xpath); if (comppath == NULL || pg_xml_error_occurred(xmlerrcxt)) + { + if (comppath != NULL) + xmlXPathFreeCompExpr(comppath); xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY, "XPath Syntax Error"); + } /* Now evaluate the path expression. */ workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt); -- 2.54.0
signature.asc
Description: PGP signature
