Re: Postgresql 8.3 beta crash

Tom Lane wrote:
> Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
>> So my current theory is:
>
>> In xmlelement(), we use ExecEvalExpr(), which in turn calls xml_parse.
>> xml_parse calls xmlCleanupParser(). But when we call ExecEvalExpr(),
>> we're in the middle of constructing an xml buffer, so calling
>> xmlCleanupBuffer() probably frees something we still need.
>
> No, your first theory is closer to the mark. What is happening is that
> xmlelement neglects to call xml_init, therefore the various stuff
> allocated by libxml is allocated using malloc(). Then xml_parse is
> called, and it *does* do xml_init(), which calls xmlMemSetup. Then
> when we return to xmlelement and start freeing stuff, libxml tries
> to use xml_pfree to free something it got from malloc().

Oh, yes, you're right.

It still feels unsafe to call ExecEvalExpr while holding on to xml
structs. It means that it's not safe for external modules to use libxml2
and call xmlMemSetup or xmlSetGenericErrorFunc themselves.

> I think that (1) we need a call to xml_init here, and hence also a
> PG_TRY block;

xml_init doesn't actually do anything that would need to be free'd in
case of error. But yeah, it does seem like a good idea to free the "text
writer" and "xml buffer" allocated at the beginning of xmlelement().
They should be allocated by xml_palloc in the current memory context,
though, and freed by the memory context reset as usual, but apparently
we don't trust that for xml document or dtd objects either.

> (2) there is a lot of stuff in xml_init that should be
> one-time-only, why does it not have an "already done" flag?

Hmm. There's the check "sizeof(char) == sizeof(xmlChar)", which in fact
should be evaluated at compile time (should that actually be an
#error?). Then there's the call to xmlSetGenericErrorFunc and
xmlMemSetup which only need to be called once. But it doesn't hurt to
call them again, and it does protect us in case a dynamically loaded
module sets them differently. And then there's the call to xmlInitParser
which does need to be called every time.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com