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

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