Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 2 Jun 2025 14:21:24 +0900
From: Michael Paquier <michael@paquier.xyz>
To: maralist86@mail.ru, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18943: Return value of a function 'xmlBufferCreate' is
 dereferenced at xpath.c:177 without checking for NUL
Message-ID: <aD001M5BUG9GwF_Y@paquier.xyz>
References: <18943-2f2a04ab03904598@postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="fM37Ye2soLBbRqCK"
Content-Disposition: inline
In-Reply-To: <18943-2f2a04ab03904598@postgresql.org>
Archived-At: <https://www.postgresql.org/message-id/aD001M5BUG9GwF_Y%40paquier.xyz>
Precedence: bulk


--fM37Ye2soLBbRqCK
Content-Type: multipart/mixed; boundary="YAfDStXc+m9JNwaW"
Content-Disposition: inline


--YAfDStXc+m9JNwaW
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Jun 01, 2025 at 07:05:25PM +0000, PG Bug reporting form wrote:
> Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177
> without checking for NULL, but it is usually checked for this function.
> The function 'xmlBufferCreate' may return NULL, and in some cases the code
> xmlStrdup(buf->content) will throw an error.

Hmm.  There are quite a few things going on here, because xpath.c
lacks in a couple of code paths a PgXmlErrorContext and we need
TRY/CATCH blocks to be able to free any memory allocated in the
context of libxml.  I think that this means a couple of calls to
pg_xml_init() and pg_xml_done() calls painted in the right places.

Another one is pgxml_result_to_text().  As cstring_to_text() does a
palloc(), it could be possible that we miss the allocation done for
xpresstr().

While going through the code, xslt_process() is actually a bit
incorrect with the handling of resstr?  If the palloc() used for the
result fails, we would lose the xmlFree() for resstr, meaning that
we'd better call cstring_to_text_with_len() in the try/catch block.

With all that, I get the attached.

Thoughts or comments?
--
Michael

--YAfDStXc+m9JNwaW
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-xml2-Fix-error-handling-in-corner-cases.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 76a4d0be904f885dc9d337fe0a9a3b0d446e6b24 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 2 Jun 2025 14:20:46 +0900
Subject: [PATCH] xml2: Fix error handling in corner cases

---
 contrib/xml2/xpath.c     | 177 ++++++++++++++++++++++++++-------------
 contrib/xml2/xslt_proc.c |  14 ++--
 2 files changed, 130 insertions(+), 61 deletions(-)

diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 23d3f332dbaa..1a570069cc85 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -122,62 +122,90 @@ pgxmlNodeSetToText(xmlNodeSetPtr nodeset,
 				   xmlChar *septagname,
 				   xmlChar *plainsep)
 {
-	xmlBufferPtr buf;
+	volatile xmlBufferPtr buf =3D NULL;
 	xmlChar    *result;
 	int			i;
+	PgXmlErrorContext *xmlerrcxt;
=20
-	buf =3D xmlBufferCreate();
+	/* spin some error handling */
+	xmlerrcxt =3D pg_xml_init(PG_XML_STRICTNESS_ALL);
=20
-	if ((toptagname !=3D NULL) && (xmlStrlen(toptagname) > 0))
+	PG_TRY();
 	{
-		xmlBufferWriteChar(buf, "<");
-		xmlBufferWriteCHAR(buf, toptagname);
-		xmlBufferWriteChar(buf, ">");
-	}
-	if (nodeset !=3D NULL)
-	{
-		for (i =3D 0; i < nodeset->nodeNr; i++)
+		buf =3D xmlBufferCreate();
+
+		if (buf =3D=3D NULL)
+			xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+						"could not allocate xmlBuffer");
+
+		if ((toptagname !=3D NULL) && (xmlStrlen(toptagname) > 0))
 		{
-			if (plainsep !=3D NULL)
+			xmlBufferWriteChar(buf, "<");
+			xmlBufferWriteCHAR(buf, toptagname);
+			xmlBufferWriteChar(buf, ">");
+		}
+		if (nodeset !=3D NULL)
+		{
+			for (i =3D 0; i < nodeset->nodeNr; i++)
 			{
-				xmlBufferWriteCHAR(buf,
-								   xmlXPathCastNodeToString(nodeset->nodeTab[i]));
-
-				/* If this isn't the last entry, write the plain sep. */
-				if (i < (nodeset->nodeNr) - 1)
-					xmlBufferWriteChar(buf, (char *) plainsep);
-			}
-			else
-			{
-				if ((septagname !=3D NULL) && (xmlStrlen(septagname) > 0))
+				if (plainsep !=3D NULL)
 				{
-					xmlBufferWriteChar(buf, "<");
-					xmlBufferWriteCHAR(buf, septagname);
-					xmlBufferWriteChar(buf, ">");
+					xmlBufferWriteCHAR(buf,
+									   xmlXPathCastNodeToString(nodeset->nodeTab[i]));
+
+					/* If this isn't the last entry, write the plain sep. */
+					if (i < (nodeset->nodeNr) - 1)
+						xmlBufferWriteChar(buf, (char *) plainsep);
 				}
-				xmlNodeDump(buf,
-							nodeset->nodeTab[i]->doc,
-							nodeset->nodeTab[i],
-							1, 0);
-
-				if ((septagname !=3D NULL) && (xmlStrlen(septagname) > 0))
+				else
 				{
-					xmlBufferWriteChar(buf, "</");
-					xmlBufferWriteCHAR(buf, septagname);
-					xmlBufferWriteChar(buf, ">");
+					if ((septagname !=3D NULL) && (xmlStrlen(septagname) > 0))
+					{
+						xmlBufferWriteChar(buf, "<");
+						xmlBufferWriteCHAR(buf, septagname);
+						xmlBufferWriteChar(buf, ">");
+					}
+					xmlNodeDump(buf,
+								nodeset->nodeTab[i]->doc,
+								nodeset->nodeTab[i],
+								1, 0);
+
+					if ((septagname !=3D NULL) && (xmlStrlen(septagname) > 0))
+					{
+						xmlBufferWriteChar(buf, "</");
+						xmlBufferWriteCHAR(buf, septagname);
+						xmlBufferWriteChar(buf, ">");
+					}
 				}
 			}
 		}
-	}
=20
-	if ((toptagname !=3D NULL) && (xmlStrlen(toptagname) > 0))
-	{
-		xmlBufferWriteChar(buf, "</");
-		xmlBufferWriteCHAR(buf, toptagname);
-		xmlBufferWriteChar(buf, ">");
+		if ((toptagname !=3D NULL) && (xmlStrlen(toptagname) > 0))
+		{
+			xmlBufferWriteChar(buf, "</");
+			xmlBufferWriteCHAR(buf, toptagname);
+			xmlBufferWriteChar(buf, ">");
+		}
+
+		result =3D xmlStrdup(buf->content);
+		if (result =3D=3D NULL)
+			xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+						"could not allocate result");
 	}
-	result =3D xmlStrdup(buf->content);
+	PG_CATCH();
+	{
+		if (buf)
+			 xmlBufferFree(buf);
+
+		pg_xml_done(xmlerrcxt, true);
+
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+
 	xmlBufferFree(buf);
+	pg_xml_done(xmlerrcxt, false);
+
 	return result;
 }
=20
@@ -438,34 +466,59 @@ pgxml_result_to_text(xmlXPathObjectPtr res,
 					 xmlChar *septag,
 					 xmlChar *plainsep)
 {
-	xmlChar    *xpresstr;
+	volatile xmlChar *xpresstr =3D NULL;
+	PgXmlErrorContext *xmlerrcxt;
 	text	   *xpres;
=20
 	if (res =3D=3D NULL)
 		return NULL;
=20
-	switch (res->type)
+	/* spin some error handling */
+	xmlerrcxt =3D pg_xml_init(PG_XML_STRICTNESS_ALL);
+
+	PG_TRY();
 	{
-		case XPATH_NODESET:
-			xpresstr =3D pgxmlNodeSetToText(res->nodesetval,
-										  toptag,
-										  septag, plainsep);
-			break;
+		switch (res->type)
+		{
+			case XPATH_NODESET:
+				xpresstr =3D pgxmlNodeSetToText(res->nodesetval,
+											  toptag,
+											  septag, plainsep);
+				break;
=20
-		case XPATH_STRING:
-			xpresstr =3D xmlStrdup(res->stringval);
-			break;
+			case XPATH_STRING:
+				xpresstr =3D xmlStrdup(res->stringval);
+				if (xpresstr =3D=3D NULL)
+					xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+								"could not allocate result");
+				break;
=20
-		default:
-			elog(NOTICE, "unsupported XQuery result: %d", res->type);
-			xpresstr =3D xmlStrdup((const xmlChar *) "<unsupported/>");
+			default:
+				elog(NOTICE, "unsupported XQuery result: %d", res->type);
+				xpresstr =3D xmlStrdup((const xmlChar *) "<unsupported/>");
+				if (xpresstr =3D=3D NULL)
+					xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+								"could not allocate result");
+		}
+
+		/* Now convert this result back to text */
+		xpres =3D cstring_to_text((char *) xpresstr);
 	}
+	PG_CATCH();
+	{
+		if (xpresstr !=3D NULL)
+			xmlFree((xmlChar *) xpresstr);
=20
-	/* Now convert this result back to text */
-	xpres =3D cstring_to_text((char *) xpresstr);
+		pg_xml_done(xmlerrcxt, true);
+
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
=20
 	/* Free various storage */
-	xmlFree(xpresstr);
+	xmlFree((xmlChar *) xpresstr);
+
+	pg_xml_done(xmlerrcxt, false);
=20
 	return xpres;
 }
@@ -671,6 +724,10 @@ xpath_table(PG_FUNCTION_ARGS)
 										rownr < res->nodesetval->nodeNr)
 									{
 										resstr =3D xmlXPathCastNodeToString(res->nodesetval->nodeTab[row=
nr]);
+										if (resstr =3D=3D NULL)
+											xml_ereport(xmlerrcxt,
+														ERROR, ERRCODE_OUT_OF_MEMORY,
+														"could not allocate result");
 										had_values =3D true;
 									}
 									else
@@ -680,11 +737,19 @@ xpath_table(PG_FUNCTION_ARGS)
=20
 								case XPATH_STRING:
 									resstr =3D xmlStrdup(res->stringval);
+									if (resstr =3D=3D NULL)
+										xml_ereport(xmlerrcxt,
+													ERROR, ERRCODE_OUT_OF_MEMORY,
+													"could not allocate result");
 									break;
=20
 								default:
 									elog(NOTICE, "unsupported XQuery result: %d", res->type);
 									resstr =3D xmlStrdup((const xmlChar *) "<unsupported/>");
+									if (resstr =3D=3D NULL)
+										xml_ereport(xmlerrcxt,
+													ERROR, ERRCODE_OUT_OF_MEMORY,
+													"could not allocate result");
 							}
=20
 							/*
diff --git a/contrib/xml2/xslt_proc.c b/contrib/xml2/xslt_proc.c
index b720d89f754a..117e3b677846 100644
--- a/contrib/xml2/xslt_proc.c
+++ b/contrib/xml2/xslt_proc.c
@@ -58,7 +58,7 @@ xslt_process(PG_FUNCTION_ARGS)
 	volatile xsltSecurityPrefsPtr xslt_sec_prefs =3D NULL;
 	volatile xsltTransformContextPtr xslt_ctxt =3D NULL;
 	volatile int resstat =3D -1;
-	xmlChar    *resstr =3D NULL;
+	volatile xmlChar *resstr =3D NULL;
 	int			reslen =3D 0;
=20
 	if (fcinfo->nargs =3D=3D 3)
@@ -141,7 +141,11 @@ xslt_process(PG_FUNCTION_ARGS)
 			xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
 						"failed to apply stylesheet");
=20
-		resstat =3D xsltSaveResultToString(&resstr, &reslen, restree, stylesheet=
);
+		resstat =3D xsltSaveResultToString((xmlChar **) &resstr, &reslen,
+										 restree, stylesheet);
+
+		if (resstat >=3D 0)
+			result =3D cstring_to_text_with_len((char *) resstr, reslen);
 	}
 	PG_CATCH();
 	{
@@ -155,6 +159,8 @@ xslt_process(PG_FUNCTION_ARGS)
 			xsltFreeStylesheet(stylesheet);
 		if (doctree !=3D NULL)
 			xmlFreeDoc(doctree);
+		if (resstr !=3D NULL)
+			xmlFree((xmlChar *) resstr);
 		xsltCleanupGlobals();
=20
 		pg_xml_done(xmlerrcxt, true);
@@ -176,10 +182,8 @@ xslt_process(PG_FUNCTION_ARGS)
 	if (resstat < 0)
 		PG_RETURN_NULL();
=20
-	result =3D cstring_to_text_with_len((char *) resstr, reslen);
-
 	if (resstr)
-		xmlFree(resstr);
+		xmlFree((xmlChar *) resstr);
=20
 	PG_RETURN_TEXT_P(result);
 #else							/* !USE_LIBXSLT */
--=20
2.49.0


--YAfDStXc+m9JNwaW--

--fM37Ye2soLBbRqCK
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmg9NNQACgkQnvQgOdby
QH2BrA/9H/hk3UQ3Ny+3kn9cdSRUyvVsveu9vUu0IIW2XQUtYCrd/Md/lAXoVbs1
nOUgTLCT/o7Qs/62Vs+FCNhurhXdny/ldquOBcEm4kax3xju7BqbmO8Zzi7G92lX
ETqKN/o7/4MxIPoCy1q/B2HTeepz80EIh2DWejawQsQv2skIRSUC5tF814mkq2VX
7VUdChYYmSEW1nC3a8+x94sFct792WPtIxFdLv5XOtXLsW+1COWIPG2BAGkXIFzC
/7DheQ6mFoT+V77i9RcJEyGd2Bmwn870qr8SYKtj5/ahiGL2FpnfHUcRvAyMTDZA
Lalcnu9deakIgX0TWKFKbL8BPn7vjGo9GHSrI7Xyd32oYqxPqMmI+jGxIAwEkR5b
ixlPgPqdAaK301pc7vRdti2cVgOOYO5knpDbcUVIDlArnErwgO0itqTzc/F/9tLQ
ezxcwE9hf5Z7ge0x+bQ7G6MZKBGin+opvcyy6WiSK0hlmit+PW28mv6MR5Ot/hNf
Xa7Em2DMHjggHn2geFRI4jYCZJ6Q6IQzLJsVfOvc/M5AhPg5tdp99/tvw8CE+i0X
0Fh3z2rxGV7uY/7+IExGl2ANb1Ctr54AACWRyNOTyy7H7C4FPvLbYLFSLMVmcmWD
pZ+B4aJdzAnnu0BSag0Dej238uIaQ6x0Az220V1tY6xcMA8x9cY=
=L/Zo
-----END PGP SIGNATURE-----

--fM37Ye2soLBbRqCK--