Re: pgxml bug (crash) in xslt_proc.c

On 11/10/2014 17:39, Tom Lane wrote:
> Mark Simonetti <marks@opalsoftware.co.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.
--