[xmlsec] Valgrind reports an invalid read that can lead to crash

[Aleksey Sanin]

Actually, never mind. I think I created a test case myself:
multiple chained XPath transforms after something like xslt
transform. Could you please test the following patch:

http://git.gnome.org/browse/xmlsec/diff/src/nodeset.c?id=8ee4fbabcd651f01c6ec1b6aef70853f27db65a8

to make sure it fixes your use case?

Thanks again for your bug report and investigation!

Aleksey


On 1/12/2010 10:06 PM, Aleksey Sanin wrote:
> Hi Frank!
>
> Thanks for report and investigation! Do you have a repro test
> case that I can look at? Unfortunately, the ownership of the
> DOM nodes is not trivial and I can't say for sure if this is
> the right change or not.
>
> Thank you in advance,
>
> Aleksey
>
> On 1/6/2010 5:21 AM, Frank Gross wrote:
>> Hi,
>>
>> I had some random crashes using the xmlsec library for signature, so I
>> did a test with valgrind that reported an invalid read (see attached
>> valgrind output). You can see that it happens in the
>> 'xmlXPathFreeNodeSet' function that is called in 'xmlSecNodeSetDestroy'.
>>
>> After some investigations, I find out that it is related to the use of
>> an XPath transformation that builds a list of 'xmlSecNodeSetPtr' in the
>> 'xmlSecXPathDataListExecute' function. In that function a new
>> 'xmlSecNodeSetPtr' is added at the end of the list but containing the
>> nodes to be signed according to the XPath expression. Unfortunately, the
>> first 'xmlSecNodeSetPtr' of that list has the 'destroyDoc' boolean set
>> to 1 that tells the 'xmlSecNodeSetDestroy' function to release the whole
>> document, and when it is the next 'xmlSecNodeSetPtr' of the list to be
>> destroyed it tries to release the node resulting of the XPath
>> expression, but they don't exist anymore because the document they
>> belong to has been released just before.
>>
>> To solve the problem I set the 'destroyDoc' of the first element to 0,
>> and put it to 1 on the last element of the list, so that the document is
>> only released at the very end. Actually, I simply add following code at
>> the end of the xmlSecXPathDataListExecute function just before the
>> return instruction.
>>
>> ...
>> if (res->destroyDoc) {
>> /* force the releasing of the document at the end of the list otherwise
>> xmlSecNodeSetDestroy can crash
>> because it will release the doc in the first node set but the following
>> ones have references to this document too
>> */
>> res->destroyDoc = 0;
>> res->prev->destroyDoc = 1;
>> }
>>
>> return(res);
>> ...
>>
>> I don't know if it is correct to do so or if there are some side effects
>> but it seems to fix my issue.
>>
>> Regards,
>> Frank
>>
>>
>>
>> _______________________________________________
>> xmlsec mailing list
>> xmlsec at aleksey.com
>> http://www.aleksey.com/mailman/listinfo/xmlsec
> _______________________________________________
> xmlsec mailing list
> xmlsec at aleksey.com
> http://www.aleksey.com/mailman/listinfo/xmlsec