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

[Aleksey Sanin]

Cool! Thanks for testing and letting me know!

Aleksey

On 1/14/2010 2:10 AM, Frank Gross wrote:
> Hi Aleksey,
>
> Sorry for the delay, but I had a lot of work.
> I applied your patch and it fixes my issue, and all my tests passed
> successfully too.
>
> Thanks,
> Frank
>
> Aleksey Sanin a écrit :
>>
>> 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
>>
>