Re: BUG #18943: Return value of a function 'xmlBufferCreate' is dereferenced at xpath.c:177 without checking for NUL

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