Re: BUG #3860: xpath crashes backend when is querying xmlagg result

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> + void
> + AtEOXact_xml(void)
> + {
> + if (LibxmlContext == NULL)
> + return;
> +
> + MemoryContextDelete(LibxmlContext);
> + LibxmlContext = NULL;
> +
> + xmlCleanupParser();
> + }

[ blink... ] Shouldn't that be the other way around? It looks to me
like xmlCleanupParser will be pfree'ing already-deleted data. Did you
test this with CLOBBER_FREED_MEMORY active?

Also, I don't see how this patch fixes what I believe to be the
fundamental problem, which is that we have code paths that invoke
libxml without having previously initialized the alloc support.
I think the sequence

if (LibxmlContext == NULL)
{
create LibxmlContext;
xmlMemSetup(...);
}

ought to be executed before *any* use of libxml stuff (which suggests
factoring it out as a subroutine...)

We also need to do some testing to see if letting the thing go until
transaction end is OK, or whether we will see unacceptable memory leaks
over long series of XML calls. (In particular I suspect that we'd
better zap the context at subtransaction abort; but even without any
error, are we sure that normal operations don't leak memory?)

One thing I was wondering about earlier today is whether libxml isn't
expecting NULL-return-on-failure from the malloc-substitute routine.
If we take control away from it unexpectedly, I wouldn't be a bit
surprised if its data structures are left corrupt. This might lead to
failures during cleanup.

I do like the idea of getting rid of the PG_TRY blocks and the
associated cleanup attempts; with the approach you're converging on
here, we need only assume that xmlCleanupParser() will get rid of
all staticly-allocated pointers and not crash, whereas right now we
are assuming a great deal of libxml stuff still works after an ereport
(which makes throwing ereport from xml_palloc even more risky).

regards, tom lane