Re: [xslt] [PATCH 0/3] Fix NULL deref through valuePop retval

[Nick Wellnhofer <wellnhofer aevum de>]

On Dec 18, 2013, at 23:20 , Jan Pokorný <jpokorny redhat com> wrote:

> I found a segfault arising from either lack of fail fast principle
> (does it make sense to try further if the template is evidently
> wrong, e.g., because of referencing undefined variables?) or because
> of allowing for NULL pointer dereferences -- in my case those were
> related to valuePop() return values.

Thanks for the report. Checking the return value of valuePop() is not really necessary because the libxml2 
XPath engine tries to guarantee that every XPath function can pop ‘nargs’ valid, non-NULL values off the 
stack:

https://git.gnome.org/browse/libxml2/tree/xpath.c#n13530

In your case this doesn’t work because of this check here:

https://git.gnome.org/browse/libxml2/tree/xpath.c#n2829

    if (ctxt->valueNr <= ctxt->valueFrame) {
        xmlXPatherror(ctxt, __FILE__, __LINE__, XPATH_STACK_ERROR);
        return (NULL);
    }

The call frames are borked because of the non-existing variable.

I think this should be fixed in libxml2. The following patch works for me but I’ll have to give the whole 
thing a closer look.

It still might be a good idea to check the return values of valuePop() as a safety measure.

Nick


diff --git a/xpath.c b/xpath.c
index 1f56b96..6b2cce5 100644
--- a/xpath.c
+++ b/xpath.c
@@ -13524,9 +13524,11 @@ xmlXPathCompOpEval(xmlXPathParserContextPtr ctxt, xmlXP
                 int frame;
 
                 frame = xmlXPathSetFrame(ctxt);
-                if (op->ch1 != -1)
+                if (op->ch1 != -1) {
                     total +=
                         xmlXPathCompOpEval(ctxt, &comp->steps[op->ch1]);
+                    CHECK_ERROR0;
+                }
                if (ctxt->valueNr < op->value) {
                    xmlGenericError(xmlGenericErrorContext,
                            "xmlXPathCompOpEval: parameter error\n");