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
