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

Michael Paquier <michael@paquier.xyz> writes:
> I'll try to have a second look at this patch in a couple of weeks.
> For now, I have added it to the next comment fest:
> https://commitfest.postgresql.org/patch/5794/
> If somebody would like to chime in, feel free.

Thanks for taking this on --- contrib/xml2 is really a mess so far as
error handling goes.  Your patch looks like an improvement, although
I do have one concern: the routines in xml.c that use an xmlerrcxt
seem to check xmlerrcxt->err_occurred pretty frequently, eg xmlStrdup
is used like this:

            doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
            if (doc->encoding == NULL || xmlerrcxt->err_occurred)
                xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
                            "could not allocate XML document");

Not sure if that's needed here.

There's more that could be looked at, if you feel like it:

xml_encode_special_chars seems to need a PG_TRY block to avoid
possibly leaking "tt".  I also wonder if it's really not possible
for xmlEncodeSpecialChars to fail and return NULL.  (xmltext()
doesn't believe that either, but maybe it's wrong too.)

The usage of cleanup_workspace seems quite a mess: one caller
uses a PG_TRY block to ensure it's called, but the rest don't.
I also find it confusing that pgxml_xpath returns a value that is
also referenced in the workspace and cleanup_workspace is responsible
for freeing.  I wonder if there's a better way to do that.

In the end of xslt_process, I wonder why

    if (resstr)
        xmlFree((xmlChar *) resstr);

isn't done before the pg_xml_done call.

            regards, tom lane