agora inbox for pgsql-hackers@postgresql.org  
help / color / mirror / Atom feed
[PATCH] Fix libxml leaks in contrib/xml2 XPath functions
5+ messages / 2 participants
[nested] [flat]

* [PATCH] Fix libxml leaks in contrib/xml2 XPath functions
@ 2026-05-31 22:01 Andrey Chernyy <andrey.cherny@tantorlabs.com>
  2026-06-01 04:04 ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 5+ messages in thread

From: Andrey Chernyy @ 2026-05-31 22:01 UTC (permalink / raw)
  To: pgsql-hackers@lists.postgresql.org

Hi,

While reviewing contrib/xml2 I found two successful-path libxml leaks in
XPath functions.

The attached series is:

  0001 Fix libxml string leak in contrib/xml2 xpath_list
  0002 Fix libxml leaks in contrib/xml2 xpath_table

In xpath_list(), the plain separator path passes the result of
xmlXPathCastNodeToString() directly to xmlBufferWriteCHAR().
xmlXPathCastNodeToString() returns a libxml-allocated xmlChar * that
has to be released with xmlFree(), while xmlBufferWriteCHAR() copies
the string rather than taking ownership.

In xpath_table(), xmlXPathCompiledEval() returns an xmlXPathObjectPtr
that has to be released with xmlXPathFreeObject().  The function also
stores libxml-allocated xmlChar strings in the values array before
calling BuildTupleFromCStrings().  BuildTupleFromCStrings() consumes
those strings by converting/copying them into the result tuple, so the
temporary libxml strings can be released after the tuple is built.

The second patch frees the XPath result object after each evaluation,
frees per-column string values after BuildTupleFromCStrings() has
consumed them, and tracks the current libxml allocations across the
existing PG_TRY block so they are also released on error.

The attached manual repro scripts exercise the two paths separately.
They are Linux-specific because they sample VmRSS from
/proc/<backend-pid>/status using pg_read_file(), so they should be run
as a superuser or a role allowed to read server files.

On unpatched origin/master, selected NOTICE lines from the repro scripts
show steady backend RSS growth:

postgres=# \i ./xml2-xpath-list-leak-repro.sql
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=1,
total_kb=38616, diff_kb=17220 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=2, total_kb=40732, diff_kb=2104
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=3,
total_kb=42720, diff_kb=1988 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=4, total_kb=44644, diff_kb=1924
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=5,
total_kb=46440, diff_kb=1796 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=6, total_kb=47968, diff_kb=1528
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=7,
total_kb=50892, diff_kb=2924 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=8, total_kb=52712, diff_kb=1820
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=9,
total_kb=54276, diff_kb=1564 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=10, total_kb=55328, diff_kb=1052

postgres=# \i ./xml2-xpath-table-leak-repro.sql
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=1,
total_kb=26452, diff_kb=5136 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=2, total_kb=27772, diff_kb=1312
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=3,
total_kb=29068, diff_kb=1296 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=4, total_kb=30368, diff_kb=1300
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=5,
total_kb=31668, diff_kb=1300 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=6, total_kb=32968, diff_kb=1300
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=7,
total_kb=34260, diff_kb=1292 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=8, total_kb=35568, diff_kb=1308
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=9,
total_kb=36876, diff_kb=1308 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=10, total_kb=38168, diff_kb=1292

With both patches applied, the same scripts plateau after the initial
allocations:

postgres=# \i ./xml2-xpath-list-leak-repro.sql
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=1,
total_kb=24480, diff_kb=3020 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=2, total_kb=24532, diff_kb=44
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=3,
total_kb=24580, diff_kb=48 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=4, total_kb=24580, diff_kb=0
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=5,
total_kb=24624, diff_kb=44 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=6, total_kb=24580, diff_kb=-44
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=7,
total_kb=24580, diff_kb=0 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=8, total_kb=24580, diff_kb=0
psql:xml2-xpath-list-leak-repro.sql:38: NOTICE:  xpath_list i=9,
total_kb=24580, diff_kb=0 psql:xml2-xpath-list-leak-repro.sql:38:
NOTICE:  xpath_list i=10, total_kb=24580, diff_kb=0

postgres=# \i ./xml2-xpath-table-leak-repro.sql
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=1,
total_kb=26572, diff_kb=188 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=2, total_kb=26600, diff_kb=28
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=3,
total_kb=26600, diff_kb=0 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=4, total_kb=26600, diff_kb=0
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=5,
total_kb=26600, diff_kb=0 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=6, total_kb=26600, diff_kb=0
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=7,
total_kb=26600, diff_kb=0 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=8, total_kb=26600, diff_kb=0
psql:xml2-xpath-table-leak-repro.sql:40: NOTICE:  xpath_table i=9,
total_kb=26600, diff_kb=0 psql:xml2-xpath-table-leak-repro.sql:40:
NOTICE:  xpath_table i=10, total_kb=26600, diff_kb=0

contrib/xml2 regression tests still pass:

    make -C contrib/xml2 check
    # All 1 tests passed.

I also checked that the two patches apply cleanly to current
origin/master with git am.

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.

--
Andrey Chernyy

Attachments:

  [application/sql] xml2-xpath-list-leak-repro.sql (1015B, ../../20260601010124.5edf9a20@andrnote/2-xml2-xpath-list-leak-repro.sql)
  download

  [application/sql] xml2-xpath-table-leak-repro.sql (1.1K, ../../20260601010124.5edf9a20@andrnote/3-xml2-xpath-table-leak-repro.sql)
  download

  [text/x-patch] 0001-Fix-libxml-string-leak-in-contrib-xml2-xpath_list.patch (1.8K, ../../20260601010124.5edf9a20@andrnote/4-0001-Fix-libxml-string-leak-in-contrib-xml2-xpath_list.patch)
  download | inline diff:
From fadb35b4b9f24aa00d3604d2885ad009791b3f3b Mon Sep 17 00:00:00 2001
From: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Date: Mon, 25 May 2026 22:12:02 +0300
Subject: [PATCH 1/2] 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 7bf477e0c3f..94819961787 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

  [text/x-patch] 0002-Fix-libxml-leaks-in-contrib-xml2-xpath_table.patch (3.6K, ../../20260601010124.5edf9a20@andrnote/5-0002-Fix-libxml-leaks-in-contrib-xml2-xpath_table.patch)
  download | inline diff:
From 3430cf722811dd256f205c3fbf3a8e16431913a0 Mon Sep 17 00:00:00 2001
From: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Date: Tue, 26 May 2026 00:11:06 +0300
Subject: [PATCH 2/2] 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 94819961787..ac140a640e0 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

^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions
  2026-05-31 22:01 [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
@ 2026-06-01 04:04 ` Michael Paquier <michael@paquier.xyz>
  2026-06-02 00:43   ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
  0 siblings, 1 reply; 5+ messages in thread

From: Michael Paquier @ 2026-06-01 04:04 UTC (permalink / raw)
  To: Andrey Chernyy <andrey.cherny@tantorlabs.com>; +Cc: pgsql-hackers@lists.postgresql.org

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 <andrey.cherny@tantorlabs.com>
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 <andrey.cherny@tantorlabs.com>
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 <michael@paquier.xyz>
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

Attachments:

  [text/plain] v2-0001-Fix-libxml-string-leak-in-contrib-xml2-xpath_list.patch (1.8K, ../../ah0E0xLvv5_OtOi6@paquier.xyz/2-v2-0001-Fix-libxml-string-leak-in-contrib-xml2-xpath_list.patch)
  download | inline diff:
From 7f0302c0e4a74b3ec3f497c00a7f853c1e0fbbd3 Mon Sep 17 00:00:00 2001
From: Andrey Chernyy <andrey.cherny@tantorlabs.com>
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

  [text/plain] v2-0002-Fix-libxml-leaks-in-contrib-xml2-xpath_table.patch (3.6K, ../../ah0E0xLvv5_OtOi6@paquier.xyz/3-v2-0002-Fix-libxml-leaks-in-contrib-xml2-xpath_table.patch)
  download | inline diff:
From e539f2f11bc892dd299ef13080e6b338202fdf9e Mon Sep 17 00:00:00 2001
From: Andrey Chernyy <andrey.cherny@tantorlabs.com>
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

  [text/plain] v2-0003-xml2-Fix-two-more-leaks.patch (1.2K, ../../ah0E0xLvv5_OtOi6@paquier.xyz/4-v2-0003-xml2-Fix-two-more-leaks.patch)
  download | inline diff:
From 6ff5a77a921eb3fdaa25e4e7884d7c1ab17a63e3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
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

  [application/pgp-signature] signature.asc (832B, ../../ah0E0xLvv5_OtOi6@paquier.xyz/5-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions
  2026-05-31 22:01 [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
  2026-06-01 04:04 ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Michael Paquier <michael@paquier.xyz>
@ 2026-06-02 00:43   ` Andrey Chernyy <andrey.cherny@tantorlabs.com>
  2026-06-02 22:10     ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 5+ messages in thread

From: Andrey Chernyy @ 2026-06-02 00:43 UTC (permalink / raw)
  To: pgsql-hackers@lists.postgresql.org


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

Attachments:

  [application/sql] xml2-pgxml-xpath-error-leak-repro.sql (1.4K, ../../20260602034314.7eadc4d7@andrnote/2-xml2-pgxml-xpath-error-leak-repro.sql)
  download

  [text/x-patch] v2-0004-xml2-Avoid-libxml-leaks-in-pgxml_xpath-error-path.patch (3.1K, ../../20260602034314.7eadc4d7@andrnote/3-v2-0004-xml2-Avoid-libxml-leaks-in-pgxml_xpath-error-path.patch)
  download | inline diff:
From fbe84d97d2256f6289d75b37735f2bbd4bc6c853 Mon Sep 17 00:00:00 2001
From: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Date: Tue, 2 Jun 2026 01:41:38 +0300
Subject: [PATCH v2 4/4] xml2: Avoid libxml leaks in pgxml_xpath() error paths

pgxml_xpath() builds an xpath_workspace before returning it to
callers.  If an ERROR is thrown before the function returns, callers
have not received the workspace pointer yet and cannot run
cleanup_workspace().

Add local error cleanup for the partially built workspace and the
compiled XPath expression.  Also check xmlXPathNewContext() before
dereferencing the returned context.
---
 contrib/xml2/xpath.c | 51 +++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 9fe75cb5ff4..283bb51178d 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -497,36 +497,49 @@ static xpath_workspace *
 pgxml_xpath(text *document, xmlChar *xpath, PgXmlErrorContext *xmlerrcxt)
 {
 	int32		docsize = VARSIZE_ANY_EXHDR(document);
-	xmlXPathCompExprPtr comppath;
+	xmlXPathCompExprPtr volatile comppath = NULL;
 	xpath_workspace *workspace = palloc0_object(xpath_workspace);
 
 	workspace->doctree = NULL;
 	workspace->ctxt = NULL;
 	workspace->res = NULL;
 
-	workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
-									   docsize, NULL, NULL,
-									   XML_PARSE_NOENT);
-	if (workspace->doctree != NULL)
+	PG_TRY();
 	{
-		workspace->ctxt = xmlXPathNewContext(workspace->doctree);
-		workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
-
-		/* compile the path */
-		comppath = xmlXPathCtxtCompile(workspace->ctxt, xpath);
-		if (comppath == NULL || pg_xml_error_occurred(xmlerrcxt))
+		workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
+										   docsize, NULL, NULL,
+										   XML_PARSE_NOENT);
+		if (workspace->doctree != NULL)
 		{
-			if (comppath != NULL)
-				xmlXPathFreeCompExpr(comppath);
-			xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
-						"XPath Syntax Error");
-		}
+			workspace->ctxt = xmlXPathNewContext(workspace->doctree);
+			if (workspace->ctxt == NULL)
+				xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+							"could not allocate XPath context");
 
-		/* Now evaluate the path expression. */
-		workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt);
+			workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
+
+			/* compile the path */
+			comppath = xmlXPathCtxtCompile(workspace->ctxt, xpath);
+			if (comppath == NULL || pg_xml_error_occurred(xmlerrcxt))
+				xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
+							"XPath Syntax Error");
+
+			/* Now evaluate the path expression. */
+			workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt);
+
+			xmlXPathFreeCompExpr(comppath);
+			comppath = NULL;
+		}
+	}
+	PG_CATCH();
+	{
+		if (comppath != NULL)
+			xmlXPathFreeCompExpr(comppath);
+		cleanup_workspace(workspace);
 
-		xmlXPathFreeCompExpr(comppath);
+		PG_RE_THROW();
 	}
+	PG_END_TRY();
 
 	return workspace;
 }
-- 
2.54.0

^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions
  2026-05-31 22:01 [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
  2026-06-01 04:04 ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Michael Paquier <michael@paquier.xyz>
  2026-06-02 00:43   ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
@ 2026-06-02 22:10     ` Michael Paquier <michael@paquier.xyz>
  2026-06-02 22:29       ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
  0 siblings, 1 reply; 5+ messages in thread

From: Michael Paquier @ 2026-06-02 22:10 UTC (permalink / raw)
  To: Andrey Chernyy <andrey.cherny@tantorlabs.com>; +Cc: pgsql-hackers@lists.postgresql.org

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

Attachments:

  [application/pgp-signature] signature.asc (832B, ../../ah9Uz-VwT9H_lpvG@paquier.xyz/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 5+ messages in thread

* Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions
  2026-05-31 22:01 [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
  2026-06-01 04:04 ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Michael Paquier <michael@paquier.xyz>
  2026-06-02 00:43   ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
  2026-06-02 22:10     ` Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Michael Paquier <michael@paquier.xyz>
@ 2026-06-02 22:29       ` Andrey Chernyy <andrey.cherny@tantorlabs.com>
  0 siblings, 0 replies; 5+ messages in thread

From: Andrey Chernyy @ 2026-06-02 22:29 UTC (permalink / raw)
  To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-hackers@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 5+ messages in thread


end of thread, other threads:[~2026-06-02 22:29 UTC | newest]

Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-31 22:01 [PATCH] Fix libxml leaks in contrib/xml2 XPath functions Andrey Chernyy <andrey.cherny@tantorlabs.com>
2026-06-01 04:04 ` Michael Paquier <michael@paquier.xyz>
2026-06-02 00:43   ` Andrey Chernyy <andrey.cherny@tantorlabs.com>
2026-06-02 22:10     ` Michael Paquier <michael@paquier.xyz>
2026-06-02 22:29       ` Andrey Chernyy <andrey.cherny@tantorlabs.com>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox