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

[Frank Gross]

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
>

-- 
Frank GROSS
Software Engineer - Web Services
Four J's Development Tools - http://www.4js.com