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

[Aleksey Sanin]

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