From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: maralist86@mail.ru, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL
In-reply-to: <aD53hVjiW-9C29VT@paquier.xyz>
References: <18943-2f2a04ab03904598@postgresql.org> <aD001M5BUG9GwF_Y@paquier.xyz> <aD53hVjiW-9C29VT@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Tue, 03 Jun 2025 13:18:13 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <861592.1748970933.1@sss.pgh.pa.us>
Date: Tue, 03 Jun 2025 13:15:33 -0400
Message-ID: <861593.1748970933@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/861593.1748970933%40sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> 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