public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: [email protected]
To: [email protected]
Subject: Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
Date: Mon, 2 Jun 2025 14:21:24 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
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
Attachments:
[text/x-diff] 0001-xml2-Fix-error-handling-in-corner-cases.patch (8.0K, 2-0001-xml2-Fix-error-handling-in-corner-cases.patch)
download | inline diff:
From 76a4d0be904f885dc9d337fe0a9a3b0d446e6b24 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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 = NULL;
xmlChar *result;
int i;
+ PgXmlErrorContext *xmlerrcxt;
- buf = xmlBufferCreate();
+ /* spin some error handling */
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- if ((toptagname != NULL) && (xmlStrlen(toptagname) > 0))
+ PG_TRY();
{
- xmlBufferWriteChar(buf, "<");
- xmlBufferWriteCHAR(buf, toptagname);
- xmlBufferWriteChar(buf, ">");
- }
- if (nodeset != NULL)
- {
- for (i = 0; i < nodeset->nodeNr; i++)
+ buf = xmlBufferCreate();
+
+ if (buf == NULL)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlBuffer");
+
+ if ((toptagname != NULL) && (xmlStrlen(toptagname) > 0))
{
- if (plainsep != NULL)
+ xmlBufferWriteChar(buf, "<");
+ xmlBufferWriteCHAR(buf, toptagname);
+ xmlBufferWriteChar(buf, ">");
+ }
+ if (nodeset != NULL)
+ {
+ for (i = 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 != NULL) && (xmlStrlen(septagname) > 0))
+ if (plainsep != 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 != NULL) && (xmlStrlen(septagname) > 0))
+ else
{
- xmlBufferWriteChar(buf, "</");
- xmlBufferWriteCHAR(buf, septagname);
- xmlBufferWriteChar(buf, ">");
+ if ((septagname != NULL) && (xmlStrlen(septagname) > 0))
+ {
+ xmlBufferWriteChar(buf, "<");
+ xmlBufferWriteCHAR(buf, septagname);
+ xmlBufferWriteChar(buf, ">");
+ }
+ xmlNodeDump(buf,
+ nodeset->nodeTab[i]->doc,
+ nodeset->nodeTab[i],
+ 1, 0);
+
+ if ((septagname != NULL) && (xmlStrlen(septagname) > 0))
+ {
+ xmlBufferWriteChar(buf, "</");
+ xmlBufferWriteCHAR(buf, septagname);
+ xmlBufferWriteChar(buf, ">");
+ }
}
}
}
- }
- if ((toptagname != NULL) && (xmlStrlen(toptagname) > 0))
- {
- xmlBufferWriteChar(buf, "</");
- xmlBufferWriteCHAR(buf, toptagname);
- xmlBufferWriteChar(buf, ">");
+ if ((toptagname != NULL) && (xmlStrlen(toptagname) > 0))
+ {
+ xmlBufferWriteChar(buf, "</");
+ xmlBufferWriteCHAR(buf, toptagname);
+ xmlBufferWriteChar(buf, ">");
+ }
+
+ result = xmlStrdup(buf->content);
+ if (result == NULL)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
}
- result = 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;
}
@@ -438,34 +466,59 @@ pgxml_result_to_text(xmlXPathObjectPtr res,
xmlChar *septag,
xmlChar *plainsep)
{
- xmlChar *xpresstr;
+ volatile xmlChar *xpresstr = NULL;
+ PgXmlErrorContext *xmlerrcxt;
text *xpres;
if (res == NULL)
return NULL;
- switch (res->type)
+ /* spin some error handling */
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
+
+ PG_TRY();
{
- case XPATH_NODESET:
- xpresstr = pgxmlNodeSetToText(res->nodesetval,
- toptag,
- septag, plainsep);
- break;
+ switch (res->type)
+ {
+ case XPATH_NODESET:
+ xpresstr = pgxmlNodeSetToText(res->nodesetval,
+ toptag,
+ septag, plainsep);
+ break;
- case XPATH_STRING:
- xpresstr = xmlStrdup(res->stringval);
- break;
+ case XPATH_STRING:
+ xpresstr = xmlStrdup(res->stringval);
+ if (xpresstr == NULL)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
+ break;
- default:
- elog(NOTICE, "unsupported XQuery result: %d", res->type);
- xpresstr = xmlStrdup((const xmlChar *) "<unsupported/>");
+ default:
+ elog(NOTICE, "unsupported XQuery result: %d", res->type);
+ xpresstr = xmlStrdup((const xmlChar *) "<unsupported/>");
+ if (xpresstr == NULL)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
+ }
+
+ /* Now convert this result back to text */
+ xpres = cstring_to_text((char *) xpresstr);
}
+ PG_CATCH();
+ {
+ if (xpresstr != NULL)
+ xmlFree((xmlChar *) xpresstr);
- /* Now convert this result back to text */
- xpres = cstring_to_text((char *) xpresstr);
+ pg_xml_done(xmlerrcxt, true);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
/* Free various storage */
- xmlFree(xpresstr);
+ xmlFree((xmlChar *) xpresstr);
+
+ pg_xml_done(xmlerrcxt, false);
return xpres;
}
@@ -671,6 +724,10 @@ xpath_table(PG_FUNCTION_ARGS)
rownr < res->nodesetval->nodeNr)
{
resstr = xmlXPathCastNodeToString(res->nodesetval->nodeTab[rownr]);
+ if (resstr == NULL)
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
had_values = true;
}
else
@@ -680,11 +737,19 @@ xpath_table(PG_FUNCTION_ARGS)
case XPATH_STRING:
resstr = xmlStrdup(res->stringval);
+ if (resstr == NULL)
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
break;
default:
elog(NOTICE, "unsupported XQuery result: %d", res->type);
resstr = xmlStrdup((const xmlChar *) "<unsupported/>");
+ if (resstr == NULL)
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
}
/*
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 = NULL;
volatile xsltTransformContextPtr xslt_ctxt = NULL;
volatile int resstat = -1;
- xmlChar *resstr = NULL;
+ volatile xmlChar *resstr = NULL;
int reslen = 0;
if (fcinfo->nargs == 3)
@@ -141,7 +141,11 @@ xslt_process(PG_FUNCTION_ARGS)
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"failed to apply stylesheet");
- resstat = xsltSaveResultToString(&resstr, &reslen, restree, stylesheet);
+ resstat = xsltSaveResultToString((xmlChar **) &resstr, &reslen,
+ restree, stylesheet);
+
+ if (resstat >= 0)
+ result = cstring_to_text_with_len((char *) resstr, reslen);
}
PG_CATCH();
{
@@ -155,6 +159,8 @@ xslt_process(PG_FUNCTION_ARGS)
xsltFreeStylesheet(stylesheet);
if (doctree != NULL)
xmlFreeDoc(doctree);
+ if (resstr != NULL)
+ xmlFree((xmlChar *) resstr);
xsltCleanupGlobals();
pg_xml_done(xmlerrcxt, true);
@@ -176,10 +182,8 @@ xslt_process(PG_FUNCTION_ARGS)
if (resstat < 0)
PG_RETURN_NULL();
- result = cstring_to_text_with_len((char *) resstr, reslen);
-
if (resstr)
- xmlFree(resstr);
+ xmlFree((xmlChar *) resstr);
PG_RETURN_TEXT_P(result);
#else /* !USE_LIBXSLT */
--
2.49.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox