Message-ID: <31f3480e-cd7d-4021-b392-87922572cc37@uni-muenster.de>
Date: Fri, 6 Jun 2025 12:22:30 +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>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org,
 maralist86@mail.ru
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>
 <d2410ca0-c0dd-4f63-9e70-3d7a62a5d705@uni-muenster.de>
 <b35e2342-0f02-4365-94cf-55052ac9bda1@uni-muenster.de>
 <aEKCoNIfLxjyKY3r@paquier.xyz>
Content-Language: en-US, de-DE
From: Jim Jones <jim.jones@uni-muenster.de>
In-Reply-To: <aEKCoNIfLxjyKY3r@paquier.xyz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/31f3480e-cd7d-4021-b392-87922572cc37%40uni-muenster.de>
Precedence: bulk

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