Date: Tue, 2 Jun 2026 03:43:14 +0300
From: Andrey Chernyy <andrey.cherny@tantorlabs.com>
To: pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Fix libxml leaks in contrib/xml2 XPath functions
Message-ID: <20260602034314.7eadc4d7@andrnote>
In-Reply-To: <ah0E0xLvv5_OtOi6@paquier.xyz>
References: <20260601010124.5edf9a20@andrnote>
	<ah0E0xLvv5_OtOi6@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/LMSk9=uMhuQ1QiIJDarDO8G"
Archived-At: 
 <https://www.postgresql.org/message-id/20260602034314.7eadc4d7%40andrnote>
Precedence: bulk

--MP_/LMSk9=uMhuQ1QiIJDarDO8G
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline


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

--MP_/LMSk9=uMhuQ1QiIJDarDO8G
Content-Type: application/sql
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename=xml2-pgxml-xpath-error-leak-repro.sql

XHNldCBPTl9FUlJPUl9TVE9QIG9uCgpDUkVBVEUgRVhURU5TSU9OIElGIE5PVCBFWElTVFMgeG1s
MjsKCkNSRUFURSBPUiBSRVBMQUNFIEZVTkNUSU9OIHBnX3RlbXAudm1yc3Nfa2IoKQpSRVRVUk5T
IGludGVnZXIKTEFOR1VBR0Ugc3FsCkFTICQkCiAgU0VMRUNUICgocmVnZXhwX21hdGNoKAogICAg
ICAgICAgICAgcGdfcmVhZF9maWxlKCcvcHJvYy8nIHx8IHBnX2JhY2tlbmRfcGlkKCkgfHwgJy9z
dGF0dXMnKSwKICAgICAgICAgICAgICdWbVJTUzpbWzpzcGFjZTpdXSsoWzAtOV0rKVtbOnNwYWNl
Ol1dK2tCJwogICAgICAgICApKVsxXSk6OmludGVnZXI7CiQkOwoKRFJPUCBUQUJMRSBJRiBFWElT
VFMgcGdfdGVtcC54bWwyX3hwYXRoX2Vycm9yX2RvYzsKCkNSRUFURSBURU1QIFRBQkxFIHhtbDJf
eHBhdGhfZXJyb3JfZG9jIEFTClNFTEVDVCAnPGRvYz4nIHx8CiAgICAgICBzdHJpbmdfYWdnKCc8
bj4nIHx8IHJlcGVhdChtZDUoZzo6dGV4dCksIDEyOCkgfHwgJzwvbj4nLCAnJykgfHwKICAgICAg
ICc8L2RvYz4nIEFTIGRvYwpGUk9NIGdlbmVyYXRlX3NlcmllcygxLCA1MTIpIEFTIGc7CgpETyAk
JApERUNMQVJFCiAgICBiZWZvcmVfcnNzIGJpZ2ludDsKICAgIGFmdGVyX3JzcyAgYmlnaW50Owog
ICAgZGlmZiAgICAgICBiaWdpbnQ7CiAgICBmYWlsdXJlcyAgIGludGVnZXI7CiAgICBlcnJjb2Rl
ICAgIHRleHQ7CkJFR0lOCiAgICBGT1IgaSBJTiAxLi4xMCBMT09QCiAgICAgICAgYmVmb3JlX3Jz
cyA6PSBwZ190ZW1wLnZtcnNzX2tiKCk7CiAgICAgICAgZmFpbHVyZXMgOj0gMDsKICAgICAgICBG
T1IgaiBJTiAxLi4yMCBMT09QCiAgICAgICAgICAgIEJFR0lOCiAgICAgICAgICAgICAgICBQRVJG
T1JNIHhwYXRoX2xpc3QoZG9jLCAnLy8qWycsICcsJykgRlJPTSB4bWwyX3hwYXRoX2Vycm9yX2Rv
YzsKICAgICAgICAgICAgRVhDRVBUSU9OIFdIRU4gb3RoZXJzIFRIRU4KICAgICAgICAgICAgICAg
IEdFVCBTVEFDS0VEIERJQUdOT1NUSUNTIGVycmNvZGUgPSBSRVRVUk5FRF9TUUxTVEFURTsKICAg
ICAgICAgICAgICAgIElGIGVycmNvZGUgPD4gJzEwNjA4JyBUSEVOCiAgICAgICAgICAgICAgICAg
ICAgUkFJU0U7CiAgICAgICAgICAgICAgICBFTkQgSUY7CiAgICAgICAgICAgICAgICBmYWlsdXJl
cyA6PSBmYWlsdXJlcyArIDE7CiAgICAgICAgICAgIEVORDsKICAgICAgICBFTkQgTE9PUDsKICAg
ICAgICBhZnRlcl9yc3MgOj0gcGdfdGVtcC52bXJzc19rYigpOwogICAgICAgIGRpZmYgOj0gYWZ0
ZXJfcnNzIC0gYmVmb3JlX3JzczsKICAgICAgICBSQUlTRSBOT1RJQ0UgJ2Vycm9yIGk9JSwgZmFp
bHVyZXM9JSwgdG90YWxfa2I9JSwgZGlmZl9rYj0lJywKICAgICAgICAgICAgICAgICAgICAgaSwg
ZmFpbHVyZXMsIGFmdGVyX3JzcywgZGlmZjsKICAgIEVORCBMT09QOwpFTkQgJCQ7Cg==

--MP_/LMSk9=uMhuQ1QiIJDarDO8G
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=v2-0004-xml2-Avoid-libxml-leaks-in-pgxml_xpath-error-path.patch

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


--MP_/LMSk9=uMhuQ1QiIJDarDO8G--