Re: pgxml bug (crash) in xslt_proc.c

On 11/10/2014 17:39, Tom Lane wrote:
> Mark Simonetti <marks(at)opalsoftware(dot)co(dot)uk> writes:
>> I hadn't really thought of it as a security issue, it came about from
>> just trying to use it normally while developing software for one of my
>> clients. At first I found it hard to repeat, but I eventually found a
>> query to repeat the problem 100% of the time. Unfortunately the XML I
>> used to repeat it is vast and generated from lots of database data so it
>> would be hard to submit that as a test case (though I can if it would
>> help by capturing the XML data into a file and sending it along with the
>> XSLT file).
> Well, it would be nice to have a test case ...

No problem, I will sort something out though it might be tomorrow now.

>
>> It seems to be to do with the order in which resources are
>> freed:
>> I changed this (xslt_proc.c, pgxml, postgres 9.3.5, line 167 onwards) : -
>> xsltFreeStylesheet(stylesheet);
>> xmlFreeDoc(restree);
>> xmlFreeDoc(doctree);
>> xsltFreeSecurityPrefs(xslt_sec_prefs);
>> xsltFreeTransformContext(xslt_ctxt); <== crash here
>> To this:
>> xsltFreeTransformContext(xslt_ctxt);
>> xsltFreeSecurityPrefs(xslt_sec_prefs);
>> xsltFreeStylesheet(stylesheet);
>> xmlFreeDoc(restree);
>> xmlFreeDoc(doctree);
>> No more crash.
> ... but this seems like a pretty straightforward change: probably the
> problem is that the xslt_ctxt has a dangling pointer to the
> xslt_sec_prefs, stylesheet, or doctree.

Yes I thought the same once I found where the problem was it seemed
fairly trivial; finding it wasn't quite so straightforward admittedly!
I'm almost certain it is doctree; xslt_sec_prefs is not touched in the
transformation context free function and stylesheet is never associated
with it (though I suppose it could be indirectly). I'm just glad I
found it and can use this build of PostgreSQL for now as the people I'm
working for are a rather large client and they want to see it working.
This is the great thing about open source though.

>
> Actually it seems to me the most sensible thing would be to free these
> various objects in reverse order of creation, which would mean that it
> ought to be
>
> xmlFreeDoc(restree);
> xsltFreeTransformContext(xslt_ctxt);
> xsltFreeSecurityPrefs(xslt_sec_prefs);
> xsltFreeStylesheet(stylesheet);
> xmlFreeDoc(doctree);
>
> Would you try that on your test case and see if it's OK?

Yep no problem, I will try that tomorrow.

Regards,
Mark.
--