public inbox for [email protected]
help / color / mirror / Atom feedRe: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
16+ messages / 5 participants
[nested] [flat]
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-02 05:21 Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2025-06-02 05:21 UTC (permalink / raw)
To: [email protected]; [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
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-03 04:18 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2025-06-03 04:18 UTC (permalink / raw)
To: [email protected]; [email protected]
On Mon, Jun 02, 2025 at 02:21:24PM +0900, Michael Paquier wrote:
> With all that, I get the attached.
>
> Thoughts or comments?
I'll try to have a second look at this patch in a couple of weeks.
For now, I have added it to the next comment fest:
https://commitfest.postgresql.org/patch/5794/
If somebody would like to chime in, feel free.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-03 17:15 Tom Lane <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Tom Lane @ 2025-06-03 17:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: [email protected]; [email protected]
Michael Paquier <[email protected]> writes:
> I'll try to have a second look at this patch in a couple of weeks.
> For now, I have added it to the next comment fest:
> https://commitfest.postgresql.org/patch/5794/
> If somebody would like to chime in, feel free.
Thanks for taking this on --- contrib/xml2 is really a mess so far as
error handling goes. Your patch looks like an improvement, although
I do have one concern: the routines in xml.c that use an xmlerrcxt
seem to check xmlerrcxt->err_occurred pretty frequently, eg xmlStrdup
is used like this:
doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
if (doc->encoding == NULL || xmlerrcxt->err_occurred)
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate XML document");
Not sure if that's needed here.
There's more that could be looked at, if you feel like it:
xml_encode_special_chars seems to need a PG_TRY block to avoid
possibly leaking "tt". I also wonder if it's really not possible
for xmlEncodeSpecialChars to fail and return NULL. (xmltext()
doesn't believe that either, but maybe it's wrong too.)
The usage of cleanup_workspace seems quite a mess: one caller
uses a PG_TRY block to ensure it's called, but the rest don't.
I also find it confusing that pgxml_xpath returns a value that is
also referenced in the workspace and cleanup_workspace is responsible
for freeing. I wonder if there's a better way to do that.
In the end of xslt_process, I wonder why
if (resstr)
xmlFree((xmlChar *) resstr);
isn't done before the pg_xml_done call.
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-05 04:52 Michael Paquier <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2025-06-05 04:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: [email protected]; [email protected]
On Tue, Jun 03, 2025 at 01:15:33PM -0400, Tom Lane wrote:
> Thanks for taking this on --- contrib/xml2 is really a mess so far as
> error handling goes. Your patch looks like an improvement, although
> I do have one concern: the routines in xml.c that use an xmlerrcxt
> seem to check xmlerrcxt->err_occurred pretty frequently, eg xmlStrdup
> is used like this:
>
> doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
> if (doc->encoding == NULL || xmlerrcxt->err_occurred)
> xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
> "could not allocate XML document");
>
> Not sure if that's needed here.
Yes, I was also wondering a bit about this part a couple of days ago
while looking at the module's code and xml.c. Looking at cacd42d62cb2
and its thread (particularly [1]), I think that we need to bite the
bullet or we are going to miss some error context.
PgXmlErrorContext can remain private in xml.c as we can reuse
pg_xml_error_occurred() to do the job for out-of-core code.
> There's more that could be looked at, if you feel like it:
Well, now that you mention these things, I do feel like it while I
have my hands on this area of the code.
> xml_encode_special_chars seems to need a PG_TRY block to avoid
> possibly leaking "tt". I also wonder if it's really not possible
> for xmlEncodeSpecialChars to fail and return NULL. (xmltext()
> doesn't believe that either, but maybe it's wrong too.)
Oh, missed this call. xmlEncodeSpecialChars() relies on
xmlEscapeText(), which can return NULL on allocation failures based
on a check in the upstream code (first argument of the routine is not
used, only second is). So xmltext() and xml_encode_special_chars()
are both wrong; we need a try/catch block for the edge case where
cstring_to_text_with_len() or cstring_to_text() could fail an
allocation or we would leak what could have been allocated by libxml.
> The usage of cleanup_workspace seems quite a mess: one caller
> uses a PG_TRY block to ensure it's called, but the rest don't.
> I also find it confusing that pgxml_xpath returns a value that is
> also referenced in the workspace and cleanup_workspace is responsible
> for freeing. I wonder if there's a better way to do that.
Yes, that's not optimal. This comes down to the fact that we need
workspace->res to not be free'd by libxml until the result of these
SQL functions is generated, and even with my previous patch we could
leak it if one of the allocations done for the function results fail.
I've been wondering about a few approaches, like adding the error
context to the workspace, but that did not feel right as we require
the error context before the try/catch block, and the workspace and
its internals allocated by libxml need to be fully handled in the
scope of the try/catch. So I've finished with the attached, where the
workspace and its cleanup routine gain volatile declarations, keeping
pg_xml_done() outside the workspace logic. This is a rather
mechanical change if done this way with the try/catch blocks moved one
level higher, but as long as we need to hold on the result inside
the workspace, I'm feeling that this is a cleaner approach in the
long-run for xml2, because it's impossible to miss what's in the scope
of the catch cleanup with the cleanup policy enforced in the
definition of cleanup_workspace().
> In the end of xslt_process, I wonder why
>
> if (resstr)
> xmlFree((xmlChar *) resstr);
>
> isn't done before the pg_xml_done call.
Good point. Fixed.
All that is rather unlikely going to be a problem in practice, so I
don't really feel a strong reason to backpatch any of that. v18 would
be OK, but we could just also wait for v19 based on how these are
unlikely going to be problems in the field.
Or it's worth worrying about a backpatch of the xml.c code paths which
are in the core backend? The xmltext() case is isolated, so this part
is not invasive.
[1]: https://www.postgresql.org/message-id/23388.1311118974%40sss.pgh.pa.us
--
Michael
Attachments:
[text/x-diff] v2-0001-xml2-Fix-error-handling-in-corner-cases.patch (20.0K, 2-v2-0001-xml2-Fix-error-handling-in-corner-cases.patch)
download | inline diff:
From aab636ac1f757b5d3b57cf71ff8597142da79e08 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 5 Jun 2025 13:50:21 +0900
Subject: [PATCH v2] xml2: Fix error handling in corner cases
---
src/backend/utils/adt/xml.c | 32 ++-
contrib/xml2/xpath.c | 413 ++++++++++++++++++++++++------------
contrib/xml2/xslt_proc.c | 26 ++-
3 files changed, 316 insertions(+), 155 deletions(-)
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index a4150bff2eae..828ba64118b0 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -529,14 +529,36 @@ xmltext(PG_FUNCTION_ARGS)
#ifdef USE_LIBXML
text *arg = PG_GETARG_TEXT_PP(0);
text *result;
- xmlChar *xmlbuf = NULL;
+ volatile xmlChar *xmlbuf = NULL;
+ PgXmlErrorContext *xmlerrcxt;
- xmlbuf = xmlEncodeSpecialChars(NULL, xml_text2xmlChar(arg));
+ /* Otherwise, we gotta spin up some error handling. */
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- Assert(xmlbuf);
+ PG_TRY();
+ {
+ xmlbuf = xmlEncodeSpecialChars(NULL, xml_text2xmlChar(arg));
+
+ if (xmlbuf == NULL || xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+
+ result = cstring_to_text_with_len((const char *) xmlbuf,
+ xmlStrlen((const xmlChar *) xmlbuf));
+ }
+ PG_CATCH();
+ {
+ if (xmlbuf)
+ xmlFree((xmlChar *) xmlbuf);
+
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ xmlFree((xmlChar *) xmlbuf);
+ pg_xml_done(xmlerrcxt, false);
- result = cstring_to_text_with_len((const char *) xmlbuf, xmlStrlen(xmlbuf));
- xmlFree(xmlbuf);
PG_RETURN_XML_P(result);
#else
NO_XML_SUPPORT();
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 23d3f332dbaa..13c4409cf888 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -51,10 +51,10 @@ static text *pgxml_result_to_text(xmlXPathObjectPtr res, xmlChar *toptag,
static xmlChar *pgxml_texttoxmlchar(text *textstring);
-static xmlXPathObjectPtr pgxml_xpath(text *document, xmlChar *xpath,
- xpath_workspace *workspace);
+static xpath_workspace *pgxml_xpath(text *document, xmlChar *xpath,
+ PgXmlErrorContext *xmlerrcxt);
-static void cleanup_workspace(xpath_workspace *workspace);
+static void cleanup_workspace(volatile xpath_workspace *workspace);
/*
@@ -89,18 +89,40 @@ xml_encode_special_chars(PG_FUNCTION_ARGS)
{
text *tin = PG_GETARG_TEXT_PP(0);
text *tout;
- xmlChar *ts,
- *tt;
+ volatile xmlChar *tt = NULL;
+ PgXmlErrorContext *xmlerrcxt;
- ts = pgxml_texttoxmlchar(tin);
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- tt = xmlEncodeSpecialChars(NULL, ts);
+ PG_TRY();
+ {
+ xmlChar *ts;
- pfree(ts);
+ ts = pgxml_texttoxmlchar(tin);
- tout = cstring_to_text((char *) tt);
+ tt = xmlEncodeSpecialChars(NULL, ts);
+ if (tt == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+ pfree(ts);
- xmlFree(tt);
+ tout = cstring_to_text((char *) tt);
+ }
+ PG_CATCH();
+ {
+ if (tt != NULL)
+ xmlFree((xmlChar *) tt);
+
+ pg_xml_done(xmlerrcxt, true);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ if (tt != NULL)
+ xmlFree((xmlChar *) tt);
+
+ pg_xml_done(xmlerrcxt, false);
PG_RETURN_TEXT_P(tout);
}
@@ -122,62 +144,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 || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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;
}
@@ -208,16 +258,29 @@ xpath_nodeset(PG_FUNCTION_ARGS)
xmlChar *septag = pgxml_texttoxmlchar(PG_GETARG_TEXT_PP(3));
xmlChar *xpath;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, toptag, septag, NULL);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- xpres = pgxml_result_to_text(res, toptag, septag, NULL);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
pfree(xpath);
@@ -240,16 +303,29 @@ xpath_list(PG_FUNCTION_ARGS)
xmlChar *plainsep = pgxml_texttoxmlchar(PG_GETARG_TEXT_PP(2));
xmlChar *xpath;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, NULL, NULL, plainsep);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- xpres = pgxml_result_to_text(res, NULL, NULL, plainsep);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
pfree(xpath);
@@ -269,8 +345,8 @@ xpath_string(PG_FUNCTION_ARGS)
xmlChar *xpath;
int32 pathsize;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
pathsize = VARSIZE_ANY_EXHDR(xpathsupp);
@@ -286,11 +362,24 @@ xpath_string(PG_FUNCTION_ARGS)
xpath[pathsize + 7] = ')';
xpath[pathsize + 8] = '\0';
- res = pgxml_xpath(document, xpath, &workspace);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- xpres = pgxml_result_to_text(res, NULL, NULL, NULL);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, NULL, NULL, NULL);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- cleanup_workspace(&workspace);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ cleanup_workspace(workspace);
pfree(xpath);
@@ -308,24 +397,38 @@ xpath_number(PG_FUNCTION_ARGS)
text *document = PG_GETARG_TEXT_PP(0);
text *xpathsupp = PG_GETARG_TEXT_PP(1); /* XPath expression */
xmlChar *xpath;
- float4 fRes;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ float4 fRes = 0.0;
+ bool isNull = false;
+ volatile xpath_workspace *workspace = NULL;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ pfree(xpath);
- pfree(xpath);
+ if (workspace->res == NULL)
+ isNull = true;
+ else
+ fRes = xmlXPathCastToNumber(workspace->res);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- if (res == NULL)
- PG_RETURN_NULL();
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- fRes = xmlXPathCastToNumber(res);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
- cleanup_workspace(&workspace);
-
- if (xmlXPathIsNaN(fRes))
+ if (isNull || xmlXPathIsNaN(fRes))
PG_RETURN_NULL();
PG_RETURN_FLOAT4(fRes);
@@ -341,21 +444,34 @@ xpath_bool(PG_FUNCTION_ARGS)
text *xpathsupp = PG_GETARG_TEXT_PP(1); /* XPath expression */
xmlChar *xpath;
int bRes;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace = NULL;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ pfree(xpath);
- pfree(xpath);
+ if (workspace->res == NULL)
+ bRes = 0;
+ else
+ bRes = xmlXPathCastToBoolean(workspace->res);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- if (res == NULL)
- PG_RETURN_BOOL(false);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- bRes = xmlXPathCastToBoolean(res);
-
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
PG_RETURN_BOOL(bRes);
}
@@ -364,62 +480,44 @@ xpath_bool(PG_FUNCTION_ARGS)
/* Core function to evaluate XPath query */
-static xmlXPathObjectPtr
-pgxml_xpath(text *document, xmlChar *xpath, xpath_workspace *workspace)
+static xpath_workspace *
+pgxml_xpath(text *document, xmlChar *xpath, PgXmlErrorContext *xmlerrcxt)
{
int32 docsize = VARSIZE_ANY_EXHDR(document);
- PgXmlErrorContext *xmlerrcxt;
xmlXPathCompExprPtr comppath;
+ xpath_workspace *workspace = (xpath_workspace *)
+ palloc0(sizeof(xpath_workspace));
workspace->doctree = NULL;
workspace->ctxt = NULL;
workspace->res = NULL;
- xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
-
- PG_TRY();
+ workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
+ docsize, NULL, NULL,
+ XML_PARSE_NOENT);
+ if (workspace->doctree != NULL)
{
- workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
- docsize, NULL, NULL,
- XML_PARSE_NOENT);
- if (workspace->doctree != NULL)
- {
- workspace->ctxt = xmlXPathNewContext(workspace->doctree);
- workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
+ workspace->ctxt = xmlXPathNewContext(workspace->doctree);
+ workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
- /* compile the path */
- comppath = xmlXPathCtxtCompile(workspace->ctxt, xpath);
- if (comppath == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
+ /* 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);
+ /* Now evaluate the path expression. */
+ workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt);
- xmlXPathFreeCompExpr(comppath);
- }
+ xmlXPathFreeCompExpr(comppath);
}
- PG_CATCH();
- {
- cleanup_workspace(workspace);
- pg_xml_done(xmlerrcxt, true);
-
- PG_RE_THROW();
- }
- PG_END_TRY();
-
- if (workspace->res == NULL)
- cleanup_workspace(workspace);
-
- pg_xml_done(xmlerrcxt, false);
-
- return workspace->res;
+ return workspace;
}
/* Clean up after processing the result of pgxml_xpath() */
static void
-cleanup_workspace(xpath_workspace *workspace)
+cleanup_workspace(volatile xpath_workspace *workspace)
{
if (workspace->res)
xmlXPathFreeObject(workspace->res);
@@ -438,34 +536,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 || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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;
}
@@ -652,7 +775,7 @@ xpath_table(PG_FUNCTION_ARGS)
/* compile the path */
comppath = xmlXPathCtxtCompile(ctxt, xpaths[j]);
- if (comppath == NULL)
+ if (comppath == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR,
ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"XPath Syntax Error");
@@ -671,6 +794,10 @@ xpath_table(PG_FUNCTION_ARGS)
rownr < res->nodesetval->nodeNr)
{
resstr = xmlXPathCastNodeToString(res->nodesetval->nodeTab[rownr]);
+ if (resstr == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
had_values = true;
}
else
@@ -680,11 +807,19 @@ xpath_table(PG_FUNCTION_ARGS)
case XPATH_STRING:
resstr = xmlStrdup(res->stringval);
+ if (resstr == NULL || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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..c8e7dd45ed5b 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)
@@ -86,7 +86,7 @@ xslt_process(PG_FUNCTION_ARGS)
VARSIZE_ANY_EXHDR(doct), NULL, NULL,
XML_PARSE_NOENT);
- if (doctree == NULL)
+ if (doctree == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing XML document");
@@ -95,14 +95,14 @@ xslt_process(PG_FUNCTION_ARGS)
VARSIZE_ANY_EXHDR(ssheet), NULL, NULL,
XML_PARSE_NOENT);
- if (ssdoc == NULL)
+ if (ssdoc == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing stylesheet as XML document");
/* After this call we need not free ssdoc separately */
stylesheet = xsltParseStylesheetDoc(ssdoc);
- if (stylesheet == NULL)
+ if (stylesheet == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"failed to parse stylesheet");
@@ -137,11 +137,15 @@ xslt_process(PG_FUNCTION_ARGS)
restree = xsltApplyStylesheetUser(stylesheet, doctree, params,
NULL, NULL, xslt_ctxt);
- if (restree == NULL)
+ if (restree == NULL || pg_xml_error_occurred(xmlerrcxt))
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);
@@ -170,17 +176,15 @@ xslt_process(PG_FUNCTION_ARGS)
xmlFreeDoc(doctree);
xsltCleanupGlobals();
+ if (resstr)
+ xmlFree((xmlChar *) resstr);
+
pg_xml_done(xmlerrcxt, false);
/* XXX this is pretty dubious, really ought to throw error instead */
if (resstat < 0)
PG_RETURN_NULL();
- result = cstring_to_text_with_len((char *) resstr, reslen);
-
- if (resstr)
- xmlFree(resstr);
-
PG_RETURN_TEXT_P(result);
#else /* !USE_LIBXSLT */
--
2.49.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-05 09:47 Jim Jones <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Jim Jones @ 2025-06-05 09:47 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
On 05.06.25 11:38, Jim Jones wrote:
>
>
> Hi Michael
>
> Am Do., 5. Juni 2025 um 10:11 Uhr schrieb Michael Paquier
> <[email protected] <mailto:[email protected]>>:
>
> On Tue, Jun 03, 2025 at 01:15:33PM -0400, Tom Lane wrote:
> > Thanks for taking this on --- contrib/xml2 is really a mess so far as
> > error handling goes. Your patch looks like an improvement, although
> > I do have one concern: the routines in xml.c that use an xmlerrcxt
> > seem to check xmlerrcxt->err_occurred pretty frequently, eg xmlStrdup
> > is used like this:
> >
> > doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
> > if (doc->encoding == NULL || xmlerrcxt->err_occurred)
> > xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
> > "could not allocate XML document");
> >
> > Not sure if that's needed here.
>
> Yes, I was also wondering a bit about this part a couple of days ago
> while looking at the module's code and xml.c. Looking at cacd42d62cb2
> and its thread (particularly [1]), I think that we need to bite the
> bullet or we are going to miss some error context.
>
> PgXmlErrorContext can remain private in xml.c as we can reuse
> pg_xml_error_occurred() to do the job for out-of-core code.
>
> > There's more that could be looked at, if you feel like it:
>
> Well, now that you mention these things, I do feel like it while I
> have my hands on this area of the code.
>
> > xml_encode_special_chars seems to need a PG_TRY block to avoid
> > possibly leaking "tt". I also wonder if it's really not possible
> > for xmlEncodeSpecialChars to fail and return NULL. (xmltext()
> > doesn't believe that either, but maybe it's wrong too.)
>
> Oh, missed this call. xmlEncodeSpecialChars() relies on
> xmlEscapeText(), which can return NULL on allocation failures based
> on a check in the upstream code (first argument of the routine is not
> used, only second is). So xmltext() and xml_encode_special_chars()
> are both wrong; we need a try/catch block for the edge case where
> cstring_to_text_with_len() or cstring_to_text() could fail an
> allocation or we would leak what could have been allocated by libxml.
>
>
> Yeah, xmlEscapeText() does return NULL in some cases[1,2] and
> xmlEncodeSpecialChars() doesn't mind.
>
> Taking a further look at xml.c I am wondering if other functions might
> also need some attention in this regard:
>
> * xmlTextWriterStartElement [3]
> * xmlTextWriterWriteAttribute [4]
> * xmlTextWriterWriteRaw [5]
> * xmlTextWriterEndAttribute [6]
>
> We're assuming they never fail. Perhaps something like this?
> ...
> nbytes = xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
> if (nbytes == -1 || xmlerrcxt->err_occurred)
> xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
> "could not allocate xmlTextWriterStartElement");
>
> Best regards, Jim
>
> 1 -
> https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlIO.c#L205 <https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlIO.c#L205;
> 2 -
> https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlIO.c#L284 <https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlIO.c#L284;
> 3 -
> https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L967 <https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L967;
> 4 -
> https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L1950 <https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L1950;
> 5 -
> https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L1323 <https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L1323;
> 6 -
> https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L1839 <https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/xmlwriter.c#L1839>...
>
Oups .. just replied to Michael.
Sorry!
Jim
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-05 14:15 Jim Jones <[email protected]>
parent: Jim Jones <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Jim Jones @ 2025-06-05 14:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
On 05.06.25 11:47, Jim Jones wrote:
> Taking a further look at xml.c I am wondering if other functions might
> also need some attention in this regard:
>
> * xmlTextWriterStartElement [3]
> * xmlTextWriterWriteAttribute [4]
> * xmlTextWriterWriteRaw [5]
> * xmlTextWriterEndAttribute [6]
>
> We're assuming they never fail. Perhaps something like this?
> ...
> nbytes = xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
> if (nbytes == -1 || xmlerrcxt->err_occurred)
> xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
> "could not allocate xmlTextWriterStartElement");
>
There is also a further xmlXPathCastNodeToString() call in xml.c at
xml_xmlnodetoxmltype() - it calls xmlNodeGetContent() and it can return
NULL.
xmlChar *str;
str = xmlXPathCastNodeToString(cur);
PG_TRY();
{
/* Here we rely on XML having the same representation as TEXT */
char *escaped = escape_xml((char *) str);
result = (xmltype *) cstring_to_text(escaped);
pfree(escaped);
}
PG_FINALLY();
{
xmlFree(str);
}
PG_END_TRY();
The function pgxmlNodeSetToText() also calls xmlXPathCastNodeToString(),
but apparently xmlBufferAdd() can handle NULL values.[1]
Best regards, Jim
1 -
https://github.com/GNOME/libxml2/blob/2b6b3945f2df548b56f2c73c490dda9781f92eb2/buf.c#L989
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-06 05:54 Michael Paquier <[email protected]>
parent: Jim Jones <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2025-06-06 05:54 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
On Thu, Jun 05, 2025 at 04:15:19PM +0200, Jim Jones wrote:
> On 05.06.25 11:47, Jim Jones wrote:
>> Taking a further look at xml.c I am wondering if other functions might
>> also need some attention in this regard:
>>
>> * xmlTextWriterStartElement [3]
>> * xmlTextWriterWriteAttribute [4]
>> * xmlTextWriterWriteRaw [5]
>> * xmlTextWriterEndAttribute [6]
It seems to me that you mean xmlTextWriterEndElement() and not
xmlTextWriterEndAttribute() for the last one.
I've been looking at the rest of the callers (this took some time),
like:
- xmlTextWriterWriteBase64, OK.
- xmlTextWriterWriteBinHex, OK.
- xmlNewStringInputStream, which is intentional in xmlPgEntityLoader()
- xmlAddChildList, where we expect valid input.
- xmlXPathCompiledEval, where valid input is expected.
- xmlXPathNewContext, which is incorrect, could fail an allocation.
- xmlReadMemory, looks OK.
- xmlBufferWriteChar, which could fail on OOM if they need to grow
memory, but let's leave these as they are; I suspect that
xmlBufferCreate() would fail anyway.
>> We're assuming they never fail. Perhaps something like this?
>> ...
>> nbytes = xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
>> if (nbytes == -1 || xmlerrcxt->err_occurred)
>> xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
>> "could not allocate xmlTextWriterStartElement");
Oh. These can return XML_ERR_NO_MEMORY as well, with more error
patterns..
> There is also a further xmlXPathCastNodeToString() call in xml.c at
> xml_xmlnodetoxmltype() - it calls xmlNodeGetContent() and it can return
> NULL.
And it's documented as a routine that returns an allocated string, so
yes, we would miss allocation failures but we should not. I think
that we should move the call of xmlXPathCastNodeToString() inside the
PG_TRY block and rely on the xmlerrcxt given by the caller to log an
error if an allocation fails, marking xmlChar *str as volatile to free
it in the finally block if required.
> The function pgxmlNodeSetToText() also calls xmlXPathCastNodeToString(),
> but apparently xmlBufferAdd() can handle NULL values.[1]
Yeah, the patch I have posted upthread gets that done better.
What do you think?
--
Michael
Attachments:
[text/x-diff] v3-0001-xml2-Fix-error-handling-in-corner-cases.patch (22.6K, 2-v3-0001-xml2-Fix-error-handling-in-corner-cases.patch)
download | inline diff:
From 6d2e680bb75d45435b704fcf4b7225697cad4da8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Fri, 6 Jun 2025 14:53:35 +0900
Subject: [PATCH v3] xml2: Fix error handling in corner cases
---
src/backend/utils/adt/xml.c | 73 +++++--
contrib/xml2/xpath.c | 418 ++++++++++++++++++++++++------------
contrib/xml2/xslt_proc.c | 26 ++-
3 files changed, 352 insertions(+), 165 deletions(-)
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index a4150bff2eae..43bc2432eb6c 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -529,14 +529,36 @@ xmltext(PG_FUNCTION_ARGS)
#ifdef USE_LIBXML
text *arg = PG_GETARG_TEXT_PP(0);
text *result;
- xmlChar *xmlbuf = NULL;
+ volatile xmlChar *xmlbuf = NULL;
+ PgXmlErrorContext *xmlerrcxt;
- xmlbuf = xmlEncodeSpecialChars(NULL, xml_text2xmlChar(arg));
+ /* Otherwise, we gotta spin up some error handling. */
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- Assert(xmlbuf);
+ PG_TRY();
+ {
+ xmlbuf = xmlEncodeSpecialChars(NULL, xml_text2xmlChar(arg));
+
+ if (xmlbuf == NULL || xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+
+ result = cstring_to_text_with_len((const char *) xmlbuf,
+ xmlStrlen((const xmlChar *) xmlbuf));
+ }
+ PG_CATCH();
+ {
+ if (xmlbuf)
+ xmlFree((xmlChar *) xmlbuf);
+
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ xmlFree((xmlChar *) xmlbuf);
+ pg_xml_done(xmlerrcxt, false);
- result = cstring_to_text_with_len((const char *) xmlbuf, xmlStrlen(xmlbuf));
- xmlFree(xmlbuf);
PG_RETURN_XML_P(result);
#else
NO_XML_SUPPORT();
@@ -931,7 +953,10 @@ xmlelement(XmlExpr *xexpr,
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate xmlTextWriter");
- xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
+ if (xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not start xml element");
forboth(arg, named_arg_strings, narg, xexpr->arg_names)
{
@@ -939,19 +964,30 @@ xmlelement(XmlExpr *xexpr,
char *argname = strVal(lfirst(narg));
if (str)
- xmlTextWriterWriteAttribute(writer,
- (xmlChar *) argname,
- (xmlChar *) str);
+ {
+ if (xmlTextWriterWriteAttribute(writer,
+ (xmlChar *) argname,
+ (xmlChar *) str) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not write xml attribute");
+ }
}
foreach(arg, arg_strings)
{
char *str = (char *) lfirst(arg);
- xmlTextWriterWriteRaw(writer, (xmlChar *) str);
+ if (xmlTextWriterWriteRaw(writer, (xmlChar *) str) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not write raw xml text");
}
- xmlTextWriterEndElement(writer);
+ if (xmlTextWriterEndElement(writer) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not end xml element");
/* we MUST do this now to flush data out to the buffer ... */
xmlFreeTextWriter(writer);
@@ -4220,20 +4256,27 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
}
else
{
- xmlChar *str;
+ volatile xmlChar *str = NULL;
- str = xmlXPathCastNodeToString(cur);
PG_TRY();
{
+ char *escaped;
+
+ str = xmlXPathCastNodeToString(cur);
+ if (str == NULL || xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+
/* Here we rely on XML having the same representation as TEXT */
- char *escaped = escape_xml((char *) str);
+ escaped = escape_xml((char *) str);
result = (xmltype *) cstring_to_text(escaped);
pfree(escaped);
}
PG_FINALLY();
{
- xmlFree(str);
+ if (str)
+ xmlFree((xmlChar *) str);
}
PG_END_TRY();
}
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 23d3f332dbaa..9345a6c2d08f 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -51,10 +51,10 @@ static text *pgxml_result_to_text(xmlXPathObjectPtr res, xmlChar *toptag,
static xmlChar *pgxml_texttoxmlchar(text *textstring);
-static xmlXPathObjectPtr pgxml_xpath(text *document, xmlChar *xpath,
- xpath_workspace *workspace);
+static xpath_workspace *pgxml_xpath(text *document, xmlChar *xpath,
+ PgXmlErrorContext *xmlerrcxt);
-static void cleanup_workspace(xpath_workspace *workspace);
+static void cleanup_workspace(volatile xpath_workspace *workspace);
/*
@@ -89,18 +89,40 @@ xml_encode_special_chars(PG_FUNCTION_ARGS)
{
text *tin = PG_GETARG_TEXT_PP(0);
text *tout;
- xmlChar *ts,
- *tt;
+ volatile xmlChar *tt = NULL;
+ PgXmlErrorContext *xmlerrcxt;
- ts = pgxml_texttoxmlchar(tin);
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- tt = xmlEncodeSpecialChars(NULL, ts);
+ PG_TRY();
+ {
+ xmlChar *ts;
- pfree(ts);
+ ts = pgxml_texttoxmlchar(tin);
- tout = cstring_to_text((char *) tt);
+ tt = xmlEncodeSpecialChars(NULL, ts);
+ if (tt == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+ pfree(ts);
- xmlFree(tt);
+ tout = cstring_to_text((char *) tt);
+ }
+ PG_CATCH();
+ {
+ if (tt != NULL)
+ xmlFree((xmlChar *) tt);
+
+ pg_xml_done(xmlerrcxt, true);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ if (tt != NULL)
+ xmlFree((xmlChar *) tt);
+
+ pg_xml_done(xmlerrcxt, false);
PG_RETURN_TEXT_P(tout);
}
@@ -122,62 +144,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 || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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;
}
@@ -208,16 +258,29 @@ xpath_nodeset(PG_FUNCTION_ARGS)
xmlChar *septag = pgxml_texttoxmlchar(PG_GETARG_TEXT_PP(3));
xmlChar *xpath;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, toptag, septag, NULL);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- xpres = pgxml_result_to_text(res, toptag, septag, NULL);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
pfree(xpath);
@@ -240,16 +303,29 @@ xpath_list(PG_FUNCTION_ARGS)
xmlChar *plainsep = pgxml_texttoxmlchar(PG_GETARG_TEXT_PP(2));
xmlChar *xpath;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, NULL, NULL, plainsep);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- xpres = pgxml_result_to_text(res, NULL, NULL, plainsep);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
pfree(xpath);
@@ -269,8 +345,8 @@ xpath_string(PG_FUNCTION_ARGS)
xmlChar *xpath;
int32 pathsize;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
pathsize = VARSIZE_ANY_EXHDR(xpathsupp);
@@ -286,11 +362,24 @@ xpath_string(PG_FUNCTION_ARGS)
xpath[pathsize + 7] = ')';
xpath[pathsize + 8] = '\0';
- res = pgxml_xpath(document, xpath, &workspace);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- xpres = pgxml_result_to_text(res, NULL, NULL, NULL);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, NULL, NULL, NULL);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- cleanup_workspace(&workspace);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ cleanup_workspace(workspace);
pfree(xpath);
@@ -308,24 +397,38 @@ xpath_number(PG_FUNCTION_ARGS)
text *document = PG_GETARG_TEXT_PP(0);
text *xpathsupp = PG_GETARG_TEXT_PP(1); /* XPath expression */
xmlChar *xpath;
- float4 fRes;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ float4 fRes = 0.0;
+ bool isNull = false;
+ volatile xpath_workspace *workspace = NULL;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ pfree(xpath);
- pfree(xpath);
+ if (workspace->res == NULL)
+ isNull = true;
+ else
+ fRes = xmlXPathCastToNumber(workspace->res);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- if (res == NULL)
- PG_RETURN_NULL();
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- fRes = xmlXPathCastToNumber(res);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
- cleanup_workspace(&workspace);
-
- if (xmlXPathIsNaN(fRes))
+ if (isNull || xmlXPathIsNaN(fRes))
PG_RETURN_NULL();
PG_RETURN_FLOAT4(fRes);
@@ -341,21 +444,34 @@ xpath_bool(PG_FUNCTION_ARGS)
text *xpathsupp = PG_GETARG_TEXT_PP(1); /* XPath expression */
xmlChar *xpath;
int bRes;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace = NULL;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ pfree(xpath);
- pfree(xpath);
+ if (workspace->res == NULL)
+ bRes = 0;
+ else
+ bRes = xmlXPathCastToBoolean(workspace->res);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- if (res == NULL)
- PG_RETURN_BOOL(false);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- bRes = xmlXPathCastToBoolean(res);
-
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
PG_RETURN_BOOL(bRes);
}
@@ -364,62 +480,44 @@ xpath_bool(PG_FUNCTION_ARGS)
/* Core function to evaluate XPath query */
-static xmlXPathObjectPtr
-pgxml_xpath(text *document, xmlChar *xpath, xpath_workspace *workspace)
+static xpath_workspace *
+pgxml_xpath(text *document, xmlChar *xpath, PgXmlErrorContext *xmlerrcxt)
{
int32 docsize = VARSIZE_ANY_EXHDR(document);
- PgXmlErrorContext *xmlerrcxt;
xmlXPathCompExprPtr comppath;
+ xpath_workspace *workspace = (xpath_workspace *)
+ palloc0(sizeof(xpath_workspace));
workspace->doctree = NULL;
workspace->ctxt = NULL;
workspace->res = NULL;
- xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
-
- PG_TRY();
+ workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
+ docsize, NULL, NULL,
+ XML_PARSE_NOENT);
+ if (workspace->doctree != NULL)
{
- workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
- docsize, NULL, NULL,
- XML_PARSE_NOENT);
- if (workspace->doctree != NULL)
- {
- workspace->ctxt = xmlXPathNewContext(workspace->doctree);
- workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
+ workspace->ctxt = xmlXPathNewContext(workspace->doctree);
+ workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
- /* compile the path */
- comppath = xmlXPathCtxtCompile(workspace->ctxt, xpath);
- if (comppath == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
+ /* 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);
+ /* Now evaluate the path expression. */
+ workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt);
- xmlXPathFreeCompExpr(comppath);
- }
+ xmlXPathFreeCompExpr(comppath);
}
- PG_CATCH();
- {
- cleanup_workspace(workspace);
- pg_xml_done(xmlerrcxt, true);
-
- PG_RE_THROW();
- }
- PG_END_TRY();
-
- if (workspace->res == NULL)
- cleanup_workspace(workspace);
-
- pg_xml_done(xmlerrcxt, false);
-
- return workspace->res;
+ return workspace;
}
/* Clean up after processing the result of pgxml_xpath() */
static void
-cleanup_workspace(xpath_workspace *workspace)
+cleanup_workspace(volatile xpath_workspace *workspace)
{
if (workspace->res)
xmlXPathFreeObject(workspace->res);
@@ -438,34 +536,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 || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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;
}
@@ -648,11 +771,16 @@ xpath_table(PG_FUNCTION_ARGS)
for (j = 0; j < numpaths; j++)
{
ctxt = xmlXPathNewContext(doctree);
+ if (ctxt == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate XPath context");
+
ctxt->node = xmlDocGetRootElement(doctree);
/* compile the path */
comppath = xmlXPathCtxtCompile(ctxt, xpaths[j]);
- if (comppath == NULL)
+ if (comppath == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR,
ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"XPath Syntax Error");
@@ -671,6 +799,10 @@ xpath_table(PG_FUNCTION_ARGS)
rownr < res->nodesetval->nodeNr)
{
resstr = xmlXPathCastNodeToString(res->nodesetval->nodeTab[rownr]);
+ if (resstr == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
had_values = true;
}
else
@@ -680,11 +812,19 @@ xpath_table(PG_FUNCTION_ARGS)
case XPATH_STRING:
resstr = xmlStrdup(res->stringval);
+ if (resstr == NULL || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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..c8e7dd45ed5b 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)
@@ -86,7 +86,7 @@ xslt_process(PG_FUNCTION_ARGS)
VARSIZE_ANY_EXHDR(doct), NULL, NULL,
XML_PARSE_NOENT);
- if (doctree == NULL)
+ if (doctree == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing XML document");
@@ -95,14 +95,14 @@ xslt_process(PG_FUNCTION_ARGS)
VARSIZE_ANY_EXHDR(ssheet), NULL, NULL,
XML_PARSE_NOENT);
- if (ssdoc == NULL)
+ if (ssdoc == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing stylesheet as XML document");
/* After this call we need not free ssdoc separately */
stylesheet = xsltParseStylesheetDoc(ssdoc);
- if (stylesheet == NULL)
+ if (stylesheet == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"failed to parse stylesheet");
@@ -137,11 +137,15 @@ xslt_process(PG_FUNCTION_ARGS)
restree = xsltApplyStylesheetUser(stylesheet, doctree, params,
NULL, NULL, xslt_ctxt);
- if (restree == NULL)
+ if (restree == NULL || pg_xml_error_occurred(xmlerrcxt))
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);
@@ -170,17 +176,15 @@ xslt_process(PG_FUNCTION_ARGS)
xmlFreeDoc(doctree);
xsltCleanupGlobals();
+ if (resstr)
+ xmlFree((xmlChar *) resstr);
+
pg_xml_done(xmlerrcxt, false);
/* XXX this is pretty dubious, really ought to throw error instead */
if (resstat < 0)
PG_RETURN_NULL();
- result = cstring_to_text_with_len((char *) resstr, reslen);
-
- if (resstr)
- xmlFree(resstr);
-
PG_RETURN_TEXT_P(result);
#else /* !USE_LIBXSLT */
--
2.49.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-06 10:22 Jim Jones <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Jim Jones @ 2025-06-06 10:22 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
On 06.06.25 07:54, Michael Paquier wrote:
> On Thu, Jun 05, 2025 at 04:15:19PM +0200, Jim Jones wrote:
>> On 05.06.25 11:47, Jim Jones wrote:
>>> Taking a further look at xml.c I am wondering if other functions might
>>> also need some attention in this regard:
>>>
>>> * xmlTextWriterStartElement [3]
>>> * xmlTextWriterWriteAttribute [4]
>>> * xmlTextWriterWriteRaw [5]
>>> * xmlTextWriterEndAttribute [6]
>
> It seems to me that you mean xmlTextWriterEndElement() and not
> xmlTextWriterEndAttribute() for the last one.
+1
The return value of the xmlTextWriter* functions are now properly evaluated.
> - xmlBufferWriteChar, which could fail on OOM if they need to grow
> memory, but let's leave these as they are; I suspect that
> xmlBufferCreate() would fail anyway.
>
I also think we're safe in this case. xmlBufferAdd() can return
XML_ERR_ARGUMENT if the xmlBuffer* or the xmlChar* are NULL, which
cannot happen in this part of the code. XML_ERR_NO_MEMORY is also
unlikely to happen, since xmlBufferWriteChar() and xmlBufferWriteCHAR()
call xmlBufferAdd() with a -1 length.
>>> We're assuming they never fail. Perhaps something like this?
>>> ...
>>> nbytes = xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
>>> if (nbytes == -1 || xmlerrcxt->err_occurred)
>>> xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
>>> "could not allocate xmlTextWriterStartElement");
>
> Oh. These can return XML_ERR_NO_MEMORY as well, with more error
> patterns..
+1
>
>> There is also a further xmlXPathCastNodeToString() call in xml.c at
>> xml_xmlnodetoxmltype() - it calls xmlNodeGetContent() and it can return
>> NULL.
>
> And it's documented as a routine that returns an allocated string, so
> yes, we would miss allocation failures but we should not. I think
> that we should move the call of xmlXPathCastNodeToString() inside the
> PG_TRY block and rely on the xmlerrcxt given by the caller to log an
> error if an allocation fails, marking xmlChar *str as volatile to free
> it in the finally block if required.
+1
>
>> The function pgxmlNodeSetToText() also calls xmlXPathCastNodeToString(),
>> but apparently xmlBufferAdd() can handle NULL values.[1]
>
> Yeah, the patch I have posted upthread gets that done better.
>
> What do you think?
Going through xml.c once again, I stumbled upon xmlAddChildList(), which
returns the last child or NULL in case of error. [1]
...
xmlAddChildList(root, content_nodes);
So, perhaps this?
if (xmlAddChildList(root, content_nodes) == NULL ||
xmlerrcxt->err_occurred)
xml_ereport(xmlerrcxt,
ERROR, ERRCODE_OUT_OF_MEMORY,
"could not add content nodes to root element");
--
Jim
1-
https://github.com/GNOME/libxml2/blob/c6206c93872fc91d98fbc61215c5618b629bf915/tree.c#L2976
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-08 02:19 Michael Paquier <[email protected]>
parent: Jim Jones <[email protected]>
0 siblings, 3 replies; 16+ messages in thread
From: Michael Paquier @ 2025-06-08 02:19 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
On Fri, Jun 06, 2025 at 12:22:30PM +0200, Jim Jones wrote:
> So, perhaps this?
>
> if (xmlAddChildList(root, content_nodes) == NULL ||
> xmlerrcxt->err_occurred)
> xml_ereport(xmlerrcxt,
> ERROR, ERRCODE_OUT_OF_MEMORY,
> "could not add content nodes to root element");
ERRCODE_INTERNAL_ERROR would be more adapted, I'm only seeing error
code paths caused by inconsistencies in the nodes.
I have updated the patches with the attached, splitting the parts for
contrib/xml2/ and the backend into two parts. These touch error paths
that are very unlikely going to be hit in practice, so let's do all
that once v19 opens for business only on HEAD.
--
Michael
Attachments:
[text/x-diff] v4-0001-Improve-error-handling-with-calls-to-libxml2.patch (4.4K, 2-v4-0001-Improve-error-handling-with-calls-to-libxml2.patch)
download | inline diff:
From 2513b80e18c2fe283faa24f541f9fb1ed799a335 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Sun, 8 Jun 2025 11:16:52 +0900
Subject: [PATCH v4 1/2] Improve error handling with calls to libxml2
This handles improvements in the backend code.
---
src/backend/utils/adt/xml.c | 78 +++++++++++++++++++++++++++++--------
1 file changed, 62 insertions(+), 16 deletions(-)
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index a4150bff2eae..2bd39b6ac4b0 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -529,14 +529,36 @@ xmltext(PG_FUNCTION_ARGS)
#ifdef USE_LIBXML
text *arg = PG_GETARG_TEXT_PP(0);
text *result;
- xmlChar *xmlbuf = NULL;
+ volatile xmlChar *xmlbuf = NULL;
+ PgXmlErrorContext *xmlerrcxt;
- xmlbuf = xmlEncodeSpecialChars(NULL, xml_text2xmlChar(arg));
+ /* Otherwise, we gotta spin up some error handling. */
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- Assert(xmlbuf);
+ PG_TRY();
+ {
+ xmlbuf = xmlEncodeSpecialChars(NULL, xml_text2xmlChar(arg));
+
+ if (xmlbuf == NULL || xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+
+ result = cstring_to_text_with_len((const char *) xmlbuf,
+ xmlStrlen((const xmlChar *) xmlbuf));
+ }
+ PG_CATCH();
+ {
+ if (xmlbuf)
+ xmlFree((xmlChar *) xmlbuf);
+
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ xmlFree((xmlChar *) xmlbuf);
+ pg_xml_done(xmlerrcxt, false);
- result = cstring_to_text_with_len((const char *) xmlbuf, xmlStrlen(xmlbuf));
- xmlFree(xmlbuf);
PG_RETURN_XML_P(result);
#else
NO_XML_SUPPORT();
@@ -770,7 +792,10 @@ xmltotext_with_options(xmltype *data, XmlOptionType xmloption_arg, bool indent)
if (oldroot != NULL)
xmlFreeNode(oldroot);
- xmlAddChildList(root, content_nodes);
+ if (xmlAddChildList(root, content_nodes) == NULL ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not append xml node list");
/*
* We use this node to insert newlines in the dump. Note: in at
@@ -931,7 +956,10 @@ xmlelement(XmlExpr *xexpr,
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate xmlTextWriter");
- xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name);
+ if (xmlTextWriterStartElement(writer, (xmlChar *) xexpr->name) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not start xml element");
forboth(arg, named_arg_strings, narg, xexpr->arg_names)
{
@@ -939,19 +967,30 @@ xmlelement(XmlExpr *xexpr,
char *argname = strVal(lfirst(narg));
if (str)
- xmlTextWriterWriteAttribute(writer,
- (xmlChar *) argname,
- (xmlChar *) str);
+ {
+ if (xmlTextWriterWriteAttribute(writer,
+ (xmlChar *) argname,
+ (xmlChar *) str) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not write xml attribute");
+ }
}
foreach(arg, arg_strings)
{
char *str = (char *) lfirst(arg);
- xmlTextWriterWriteRaw(writer, (xmlChar *) str);
+ if (xmlTextWriterWriteRaw(writer, (xmlChar *) str) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not write raw xml text");
}
- xmlTextWriterEndElement(writer);
+ if (xmlTextWriterEndElement(writer) < 0 ||
+ xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
+ "could not end xml element");
/* we MUST do this now to flush data out to the buffer ... */
xmlFreeTextWriter(writer);
@@ -4220,20 +4259,27 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
}
else
{
- xmlChar *str;
+ volatile xmlChar *str = NULL;
- str = xmlXPathCastNodeToString(cur);
PG_TRY();
{
+ char *escaped;
+
+ str = xmlXPathCastNodeToString(cur);
+ if (str == NULL || xmlerrcxt->err_occurred)
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+
/* Here we rely on XML having the same representation as TEXT */
- char *escaped = escape_xml((char *) str);
+ escaped = escape_xml((char *) str);
result = (xmltype *) cstring_to_text(escaped);
pfree(escaped);
}
PG_FINALLY();
{
- xmlFree(str);
+ if (str)
+ xmlFree((xmlChar *) str);
}
PG_END_TRY();
}
--
2.49.0
[text/x-diff] v4-0002-xml2-Improve-error-handling-in-corner-cases.patch (19.0K, 3-v4-0002-xml2-Improve-error-handling-in-corner-cases.patch)
download | inline diff:
From a23b3707368a4db2cce53d219b7adf639d7003a5 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Sun, 8 Jun 2025 11:17:18 +0900
Subject: [PATCH v4 2/2] xml2: Improve error handling in corner cases
---
contrib/xml2/xpath.c | 418 ++++++++++++++++++++++++++-------------
contrib/xml2/xslt_proc.c | 26 +--
2 files changed, 294 insertions(+), 150 deletions(-)
diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index 23d3f332dbaa..9345a6c2d08f 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -51,10 +51,10 @@ static text *pgxml_result_to_text(xmlXPathObjectPtr res, xmlChar *toptag,
static xmlChar *pgxml_texttoxmlchar(text *textstring);
-static xmlXPathObjectPtr pgxml_xpath(text *document, xmlChar *xpath,
- xpath_workspace *workspace);
+static xpath_workspace *pgxml_xpath(text *document, xmlChar *xpath,
+ PgXmlErrorContext *xmlerrcxt);
-static void cleanup_workspace(xpath_workspace *workspace);
+static void cleanup_workspace(volatile xpath_workspace *workspace);
/*
@@ -89,18 +89,40 @@ xml_encode_special_chars(PG_FUNCTION_ARGS)
{
text *tin = PG_GETARG_TEXT_PP(0);
text *tout;
- xmlChar *ts,
- *tt;
+ volatile xmlChar *tt = NULL;
+ PgXmlErrorContext *xmlerrcxt;
- ts = pgxml_texttoxmlchar(tin);
+ xmlerrcxt = pg_xml_init(PG_XML_STRICTNESS_ALL);
- tt = xmlEncodeSpecialChars(NULL, ts);
+ PG_TRY();
+ {
+ xmlChar *ts;
- pfree(ts);
+ ts = pgxml_texttoxmlchar(tin);
- tout = cstring_to_text((char *) tt);
+ tt = xmlEncodeSpecialChars(NULL, ts);
+ if (tt == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate xmlChar");
+ pfree(ts);
- xmlFree(tt);
+ tout = cstring_to_text((char *) tt);
+ }
+ PG_CATCH();
+ {
+ if (tt != NULL)
+ xmlFree((xmlChar *) tt);
+
+ pg_xml_done(xmlerrcxt, true);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ if (tt != NULL)
+ xmlFree((xmlChar *) tt);
+
+ pg_xml_done(xmlerrcxt, false);
PG_RETURN_TEXT_P(tout);
}
@@ -122,62 +144,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 || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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;
}
@@ -208,16 +258,29 @@ xpath_nodeset(PG_FUNCTION_ARGS)
xmlChar *septag = pgxml_texttoxmlchar(PG_GETARG_TEXT_PP(3));
xmlChar *xpath;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, toptag, septag, NULL);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- xpres = pgxml_result_to_text(res, toptag, septag, NULL);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
pfree(xpath);
@@ -240,16 +303,29 @@ xpath_list(PG_FUNCTION_ARGS)
xmlChar *plainsep = pgxml_texttoxmlchar(PG_GETARG_TEXT_PP(2));
xmlChar *xpath;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, NULL, NULL, plainsep);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- xpres = pgxml_result_to_text(res, NULL, NULL, plainsep);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
pfree(xpath);
@@ -269,8 +345,8 @@ xpath_string(PG_FUNCTION_ARGS)
xmlChar *xpath;
int32 pathsize;
text *xpres;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace;
+ PgXmlErrorContext *xmlerrcxt;
pathsize = VARSIZE_ANY_EXHDR(xpathsupp);
@@ -286,11 +362,24 @@ xpath_string(PG_FUNCTION_ARGS)
xpath[pathsize + 7] = ')';
xpath[pathsize + 8] = '\0';
- res = pgxml_xpath(document, xpath, &workspace);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- xpres = pgxml_result_to_text(res, NULL, NULL, NULL);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ xpres = pgxml_result_to_text(workspace->res, NULL, NULL, NULL);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- cleanup_workspace(&workspace);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+
+ cleanup_workspace(workspace);
pfree(xpath);
@@ -308,24 +397,38 @@ xpath_number(PG_FUNCTION_ARGS)
text *document = PG_GETARG_TEXT_PP(0);
text *xpathsupp = PG_GETARG_TEXT_PP(1); /* XPath expression */
xmlChar *xpath;
- float4 fRes;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ float4 fRes = 0.0;
+ bool isNull = false;
+ volatile xpath_workspace *workspace = NULL;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ pfree(xpath);
- pfree(xpath);
+ if (workspace->res == NULL)
+ isNull = true;
+ else
+ fRes = xmlXPathCastToNumber(workspace->res);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- if (res == NULL)
- PG_RETURN_NULL();
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- fRes = xmlXPathCastToNumber(res);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
- cleanup_workspace(&workspace);
-
- if (xmlXPathIsNaN(fRes))
+ if (isNull || xmlXPathIsNaN(fRes))
PG_RETURN_NULL();
PG_RETURN_FLOAT4(fRes);
@@ -341,21 +444,34 @@ xpath_bool(PG_FUNCTION_ARGS)
text *xpathsupp = PG_GETARG_TEXT_PP(1); /* XPath expression */
xmlChar *xpath;
int bRes;
- xmlXPathObjectPtr res;
- xpath_workspace workspace;
+ volatile xpath_workspace *workspace = NULL;
+ PgXmlErrorContext *xmlerrcxt;
xpath = pgxml_texttoxmlchar(xpathsupp);
+ xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
- res = pgxml_xpath(document, xpath, &workspace);
+ PG_TRY();
+ {
+ workspace = pgxml_xpath(document, xpath, xmlerrcxt);
+ pfree(xpath);
- pfree(xpath);
+ if (workspace->res == NULL)
+ bRes = 0;
+ else
+ bRes = xmlXPathCastToBoolean(workspace->res);
+ }
+ PG_CATCH();
+ {
+ if (workspace)
+ cleanup_workspace(workspace);
- if (res == NULL)
- PG_RETURN_BOOL(false);
+ pg_xml_done(xmlerrcxt, true);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
- bRes = xmlXPathCastToBoolean(res);
-
- cleanup_workspace(&workspace);
+ cleanup_workspace(workspace);
+ pg_xml_done(xmlerrcxt, false);
PG_RETURN_BOOL(bRes);
}
@@ -364,62 +480,44 @@ xpath_bool(PG_FUNCTION_ARGS)
/* Core function to evaluate XPath query */
-static xmlXPathObjectPtr
-pgxml_xpath(text *document, xmlChar *xpath, xpath_workspace *workspace)
+static xpath_workspace *
+pgxml_xpath(text *document, xmlChar *xpath, PgXmlErrorContext *xmlerrcxt)
{
int32 docsize = VARSIZE_ANY_EXHDR(document);
- PgXmlErrorContext *xmlerrcxt;
xmlXPathCompExprPtr comppath;
+ xpath_workspace *workspace = (xpath_workspace *)
+ palloc0(sizeof(xpath_workspace));
workspace->doctree = NULL;
workspace->ctxt = NULL;
workspace->res = NULL;
- xmlerrcxt = pgxml_parser_init(PG_XML_STRICTNESS_LEGACY);
-
- PG_TRY();
+ workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
+ docsize, NULL, NULL,
+ XML_PARSE_NOENT);
+ if (workspace->doctree != NULL)
{
- workspace->doctree = xmlReadMemory((char *) VARDATA_ANY(document),
- docsize, NULL, NULL,
- XML_PARSE_NOENT);
- if (workspace->doctree != NULL)
- {
- workspace->ctxt = xmlXPathNewContext(workspace->doctree);
- workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
+ workspace->ctxt = xmlXPathNewContext(workspace->doctree);
+ workspace->ctxt->node = xmlDocGetRootElement(workspace->doctree);
- /* compile the path */
- comppath = xmlXPathCtxtCompile(workspace->ctxt, xpath);
- if (comppath == NULL)
- xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
+ /* 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);
+ /* Now evaluate the path expression. */
+ workspace->res = xmlXPathCompiledEval(comppath, workspace->ctxt);
- xmlXPathFreeCompExpr(comppath);
- }
+ xmlXPathFreeCompExpr(comppath);
}
- PG_CATCH();
- {
- cleanup_workspace(workspace);
- pg_xml_done(xmlerrcxt, true);
-
- PG_RE_THROW();
- }
- PG_END_TRY();
-
- if (workspace->res == NULL)
- cleanup_workspace(workspace);
-
- pg_xml_done(xmlerrcxt, false);
-
- return workspace->res;
+ return workspace;
}
/* Clean up after processing the result of pgxml_xpath() */
static void
-cleanup_workspace(xpath_workspace *workspace)
+cleanup_workspace(volatile xpath_workspace *workspace)
{
if (workspace->res)
xmlXPathFreeObject(workspace->res);
@@ -438,34 +536,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 || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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;
}
@@ -648,11 +771,16 @@ xpath_table(PG_FUNCTION_ARGS)
for (j = 0; j < numpaths; j++)
{
ctxt = xmlXPathNewContext(doctree);
+ if (ctxt == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate XPath context");
+
ctxt->node = xmlDocGetRootElement(doctree);
/* compile the path */
comppath = xmlXPathCtxtCompile(ctxt, xpaths[j]);
- if (comppath == NULL)
+ if (comppath == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR,
ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"XPath Syntax Error");
@@ -671,6 +799,10 @@ xpath_table(PG_FUNCTION_ARGS)
rownr < res->nodesetval->nodeNr)
{
resstr = xmlXPathCastNodeToString(res->nodesetval->nodeTab[rownr]);
+ if (resstr == NULL || pg_xml_error_occurred(xmlerrcxt))
+ xml_ereport(xmlerrcxt,
+ ERROR, ERRCODE_OUT_OF_MEMORY,
+ "could not allocate result");
had_values = true;
}
else
@@ -680,11 +812,19 @@ xpath_table(PG_FUNCTION_ARGS)
case XPATH_STRING:
resstr = xmlStrdup(res->stringval);
+ if (resstr == NULL || pg_xml_error_occurred(xmlerrcxt))
+ 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 || pg_xml_error_occurred(xmlerrcxt))
+ 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..c8e7dd45ed5b 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)
@@ -86,7 +86,7 @@ xslt_process(PG_FUNCTION_ARGS)
VARSIZE_ANY_EXHDR(doct), NULL, NULL,
XML_PARSE_NOENT);
- if (doctree == NULL)
+ if (doctree == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing XML document");
@@ -95,14 +95,14 @@ xslt_process(PG_FUNCTION_ARGS)
VARSIZE_ANY_EXHDR(ssheet), NULL, NULL,
XML_PARSE_NOENT);
- if (ssdoc == NULL)
+ if (ssdoc == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_XML_DOCUMENT,
"error parsing stylesheet as XML document");
/* After this call we need not free ssdoc separately */
stylesheet = xsltParseStylesheetDoc(ssdoc);
- if (stylesheet == NULL)
+ if (stylesheet == NULL || pg_xml_error_occurred(xmlerrcxt))
xml_ereport(xmlerrcxt, ERROR, ERRCODE_INVALID_ARGUMENT_FOR_XQUERY,
"failed to parse stylesheet");
@@ -137,11 +137,15 @@ xslt_process(PG_FUNCTION_ARGS)
restree = xsltApplyStylesheetUser(stylesheet, doctree, params,
NULL, NULL, xslt_ctxt);
- if (restree == NULL)
+ if (restree == NULL || pg_xml_error_occurred(xmlerrcxt))
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);
@@ -170,17 +176,15 @@ xslt_process(PG_FUNCTION_ARGS)
xmlFreeDoc(doctree);
xsltCleanupGlobals();
+ if (resstr)
+ xmlFree((xmlChar *) resstr);
+
pg_xml_done(xmlerrcxt, false);
/* XXX this is pretty dubious, really ought to throw error instead */
if (resstat < 0)
PG_RETURN_NULL();
- result = cstring_to_text_with_len((char *) resstr, reslen);
-
- if (resstr)
- xmlFree(resstr);
-
PG_RETURN_TEXT_P(result);
#else /* !USE_LIBXSLT */
--
2.49.0
[application/pgp-signature] signature.asc (833B, 4-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-08 08:33 Jim Jones <[email protected]>
parent: Michael Paquier <[email protected]>
2 siblings, 1 reply; 16+ messages in thread
From: Jim Jones @ 2025-06-08 08:33 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
On 08.06.25 04:19, Michael Paquier wrote:
> ERRCODE_INTERNAL_ERROR would be more adapted, I'm only seeing error
> code paths caused by inconsistencies in the nodes.
+1
> I have updated the patches with the attached, splitting the parts for
> contrib/xml2/ and the backend into two parts. These touch error paths
> that are very unlikely going to be hit in practice, so let's do all
> that once v19 opens for business only on HEAD.
Out of curiosity, why aren't we applying this to v18?
Overall, LGTM.
Thanks!
--
Jim
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-08 15:40 Tom Lane <[email protected]>
parent: Jim Jones <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: Tom Lane @ 2025-06-08 15:40 UTC (permalink / raw)
To: Jim Jones <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]; [email protected]
Jim Jones <[email protected]> writes:
> Out of curiosity, why aren't we applying this to v18?
Our risk-aversion level rises steadily over the course of a release
cycle, and is pretty high post beta1. While I think the problems
we're trying to fix here are real, they are very low-probability
(I don't recall hearing any field reports traceable to this).
And you have to remember there is also some risk of introducing
new bugs. On balance it's not a change I would back-patch, and
at this point v18 is pretty close to being a stable branch so
it's not getting fixes we wouldn't back-patch, unless that's
because they are in new-in-18 code.
tl;dr: I agree with Michael's choice to hold it for v19.
It's a judgment call of course, but I think it's the right one.
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-06-08 17:00 Jim Jones <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Jim Jones @ 2025-06-08 17:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]; [email protected]
On 08.06.25 17:40, Tom Lane wrote:
> Our risk-aversion level rises steadily over the course of a release
> cycle, and is pretty high post beta1. While I think the problems
> we're trying to fix here are real, they are very low-probability
> (I don't recall hearing any field reports traceable to this).
> And you have to remember there is also some risk of introducing
> new bugs. On balance it's not a change I would back-patch, and
> at this point v18 is pretty close to being a stable branch so
> it's not getting fixes we wouldn't back-patch, unless that's
> because they are in new-in-18 code.
Makes sense. Thanks for the clarification!
Best regards, Jim
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
@ 2025-07-08 09:49 Robin Haberkorn <[email protected]>
parent: Michael Paquier <[email protected]>
2 siblings, 0 replies; 16+ messages in thread
From: Robin Haberkorn @ 2025-07-08 09:49 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Jim Jones <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; [email protected]
Hello Michael!
On Sun Jun 8, 2025 at 05:19:29 GMT +03, Michael Paquier wrote:
> I have updated the patches with the attached, splitting the parts for
> contrib/xml2/ and the backend into two parts. These touch error paths
> that are very unlikely going to be hit in practice, so let's do all
> that once v19 opens for business only on HEAD.
I know this has already been committed, but why are we still using
PG_XML_STRICTNESS_LEGACY in xpath.c? As we are always checking
pg_xml_error_occurred() this should no longer be necessary.
It's of course also still in xslt_proc.c, but I have submitted a
separate patch to the ongoing Commitfest, which will resolve that. [1]
Best regards,
Robin Haberkorn
[1] https://commitfest.postgresql.org/patch/5718/
btw. it's still looking for a rewiever.
--
Robin Haberkorn
Software Engineer
B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / https://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt, HRB 3537
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' isdereferenced at xpath.c:177 without checking for NUL
@ 2026-03-08 14:12 =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
parent: Michael Paquier <[email protected]>
2 siblings, 1 reply; 16+ messages in thread
From: =?utf-8?B?Y2NhNTUwNw==?= @ 2026-03-08 14:12 UTC (permalink / raw)
To: =?utf-8?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; =?utf-8?B?SmltIEpvbmVz?= <[email protected]>; +Cc: =?utf-8?B?VG9tIExhbmU=?= <[email protected]>; =?utf-8?B?cGdzcWwtYnVncw==?= <[email protected]>; =?utf-8?B?bWFyYWxpc3Q4Ng==?= <[email protected]>
Hi,
It seems that there are 2 misuse of "volatile" in xml.c:
1) xmltext()
volatile xmlChar *xmlbuf = NULL; // -> xmlChar *volatile xmlbuf = NULL;
2) xml_xmlnodetoxmltype()
volatile xmlChar *str = NULL; // -> xmlChar *volatile str = NULL;
We want the pointer itself be volatile rather than what it points to.
--
Regards,
ChangAo Chen
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' isdereferenced at xpath.c:177 without checking for NUL
@ 2026-03-08 15:05 =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
parent: =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 1 reply; 16+ messages in thread
From: =?utf-8?B?Y2NhNTUwNw==?= @ 2026-03-08 15:05 UTC (permalink / raw)
To: =?utf-8?B?TWljaGFlbCBQYXF1aWVy?= <[email protected]>; =?utf-8?B?SmltIEpvbmVz?= <[email protected]>; +Cc: =?utf-8?B?VG9tIExhbmU=?= <[email protected]>; =?utf-8?B?cGdzcWwtYnVncw==?= <[email protected]>; =?utf-8?B?bWFyYWxpc3Q4Ng==?= <[email protected]>
> It seems that there are 2 misuse of "volatile" in xml.c:
>
> 1) xmltext()
>
> volatile xmlChar *xmlbuf = NULL; // -> xmlChar *volatile xmlbuf = NULL;
>
> 2) xml_xmlnodetoxmltype()
>
> volatile xmlChar *str = NULL; // -> xmlChar *volatile str = NULL;
>
> We want the pointer itself be volatile rather than what it points to.
Attach a small patch.
--
Regards,
ChangAo Chen
Attachments:
[application/octet-stream] Fix-misuse-of-volatile.patch (1.3K, 2-Fix-misuse-of-volatile.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 2c8d5a81b75..dd3270a0782 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -529,7 +529,7 @@ xmltext(PG_FUNCTION_ARGS)
#ifdef USE_LIBXML
text *arg = PG_GETARG_TEXT_PP(0);
text *result;
- volatile xmlChar *xmlbuf = NULL;
+ xmlChar *volatile xmlbuf = NULL;
PgXmlErrorContext *xmlerrcxt;
/* First we gotta spin up some error handling. */
@@ -544,19 +544,19 @@ xmltext(PG_FUNCTION_ARGS)
"could not allocate xmlChar");
result = cstring_to_text_with_len((const char *) xmlbuf,
- xmlStrlen((const xmlChar *) xmlbuf));
+ xmlStrlen(xmlbuf));
}
PG_CATCH();
{
if (xmlbuf)
- xmlFree((xmlChar *) xmlbuf);
+ xmlFree(xmlbuf);
pg_xml_done(xmlerrcxt, true);
PG_RE_THROW();
}
PG_END_TRY();
- xmlFree((xmlChar *) xmlbuf);
+ xmlFree(xmlbuf);
pg_xml_done(xmlerrcxt, false);
PG_RETURN_XML_P(result);
@@ -4247,7 +4247,7 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
}
else
{
- volatile xmlChar *str = NULL;
+ xmlChar *volatile str = NULL;
PG_TRY();
{
@@ -4267,7 +4267,7 @@ xml_xmlnodetoxmltype(xmlNodePtr cur, PgXmlErrorContext *xmlerrcxt)
PG_FINALLY();
{
if (str)
- xmlFree((xmlChar *) str);
+ xmlFree(str);
}
PG_END_TRY();
}
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #18943: Return value of a function 'xmlBufferCreate' isdereferenced at xpath.c:177 without checking for NUL
@ 2026-03-09 07:31 Michael Paquier <[email protected]>
parent: =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: Michael Paquier @ 2026-03-09 07:31 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: Jim Jones <[email protected]>; Tom Lane <[email protected]>; pgsql-bugs <[email protected]>; maralist86 <[email protected]>
On Sun, Mar 08, 2026 at 11:05:32PM +0800, cca5507 wrote:
> Attach a small patch.
You are right, this needs to consider the pointer variable as
volatile, as done in your patch, and not treat as volatile what is
pointed at. This comes from 2e947217474c, as of HEAD. I'll take care
of it later. The same business has been fixed in xml2 as of
93001888d85c.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-03-09 07:31 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-06-02 05:21 Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL Michael Paquier <[email protected]>
2025-06-03 04:18 ` Michael Paquier <[email protected]>
2025-06-03 17:15 ` Tom Lane <[email protected]>
2025-06-05 04:52 ` Michael Paquier <[email protected]>
2025-06-05 09:47 ` Jim Jones <[email protected]>
2025-06-05 14:15 ` Jim Jones <[email protected]>
2025-06-06 05:54 ` Michael Paquier <[email protected]>
2025-06-06 10:22 ` Jim Jones <[email protected]>
2025-06-08 02:19 ` Michael Paquier <[email protected]>
2025-06-08 08:33 ` Jim Jones <[email protected]>
2025-06-08 15:40 ` Tom Lane <[email protected]>
2025-06-08 17:00 ` Jim Jones <[email protected]>
2025-07-08 09:49 ` Robin Haberkorn <[email protected]>
2026-03-08 14:12 ` Re: BUG #18943: Return value of a function 'xmlBufferCreate' isdereferenced at xpath.c:177 without checking for NUL =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
2026-03-08 15:05 ` Re: BUG #18943: Return value of a function 'xmlBufferCreate' isdereferenced at xpath.c:177 without checking for NUL =?utf-8?B?Y2NhNTUwNw==?= <[email protected]>
2026-03-09 07:31 ` Re: BUG #18943: Return value of a function 'xmlBufferCreate' isdereferenced at xpath.c:177 without checking for NUL Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox