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
