Message-ID: <d2410ca0-c0dd-4f63-9e70-3d7a62a5d705@uni-muenster.de>
Date: Thu, 5 Jun 2025 11:47:01 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #18943: Return value of a function 'xmlBufferCreate' is
 dereferenced at xpath.c:177 without checking for NUL
To: Michael Paquier <michael@paquier.xyz>
References: <18943-2f2a04ab03904598@postgresql.org>
 <aD001M5BUG9GwF_Y@paquier.xyz> <aD53hVjiW-9C29VT@paquier.xyz>
 <861593.1748970933@sss.pgh.pa.us> <aEEingzOta_S_Nu7@paquier.xyz>
 <CAPLXN34Dr3Gbi+xJ6BgCeTyBJkMVe3cn7qxoADV72rC9ZHeBtQ@mail.gmail.com>
Content-Language: en-US, de-DE
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org,
 maralist86@mail.ru
From: Jim Jones <jim.jones@uni-muenster.de>
In-Reply-To: <CAPLXN34Dr3Gbi+xJ6BgCeTyBJkMVe3cn7qxoADV72rC9ZHeBtQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/d2410ca0-c0dd-4f63-9e70-3d7a62a5d705%40uni-muenster.de>
Precedence: bulk

On 05.06.25 11:38, Jim Jones wrote:
> 
> 
> Hi Michael
> 
> Am Do., 5. Juni 2025 um 10:11 Uhr schrieb Michael Paquier
> <michael@paquier.xyz <mailto:michael@paquier.xyz>>:
> 
>     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