103295 - repeatable crash using "select" attribute in apply-templates

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Paul Carr]

From Bugzilla Helper:
User-Agent: Mozilla/4.77C-CCK-MCD {C-UDP; EBM-APPLE} (Macintosh; U; PPC)
BuildID:    20011091311

Mac Mozilla 0.9.4 repeatably crashes when I attempt to load an xsl stylesheet 
using the select attribute in xsl:apply-templates, i.e. when I change this:

<xsl:apply-templates/>

to this:

<xsl:apply-templates select=".///">

or several other possibilites, so long as the select attribute is specified.

Reproducible: Always
Steps to Reproduce:
1.download bug-test.xml and event-template.xsl (note: Earthlink serves .xsl as 
text/plain)
2.Open bug-test.xml with Mozilla
3.

Actual Results:  immediate crash.

Expected Results:  processed stylesheet

Talkback ID: TB36275384E, among many others

Comment 1

[Axel Hecht]

This crashes in XSLTProcessor.cpp:1067 :
                if ( selectAtt.length() == 0 ) selectAtt = "node()";
                pExpr = ps->getPatternExpr(selectAtt);
                ExprResult* exprResult = pExpr->evaluate(node, ps);
getPatternExpr looks odd to me, anyway, but
Expr* ExprParser::createPatternExpr(const String& pattern) {
    ExprLexer lexer(pattern);
    Expr* expr = createUnionExpr(lexer);
    return expr;
} //-- createPatternExpr

needs to cope with the the error, and create our infamous StringExpr.

Axel

Comment 2

[Peter Van der Beken [:peterv]]

God, our error reporting sucks. Looking for r/sr, I would like this in 0.9.5 and
if still possible in the branch.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Proposed fix

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

You don't check for out of memory condition after:

pExpr = new ErrorFunctionCall(err);

In XSLTProcessor.cpp you had pExpr, but the patch changed the usage to expr, was
that intended (I did not look at the context to see if there is any difference,
but there might).

Please describe the importance of this feature; any examples in prominent XSLT
books, any XSLT sites out there using it etc. 

If you think this should be on the branch you are free to nominate, of course.
If the reasons are good, I think this might still make it - the patch looks safe
to me (better get real r & sr ASAP).

Comment 5

[Axel Hecht]

Comment on attachment 52219 [details] [diff] [review]
Proposed fix

r=[e-mail], though I can't build it.
Heikki, the stuff in ProcessorState fixes a potential crash
on parsing errors.
The stuff in XSLTProcessor fixes a conformance bug.
select="foo" is supposed to be an expression,
not a pattern.
That's the reason for the change of variable name. Both expr and pExpr (p for pattern) are declared, so no prob.

Thanx Peter, and all praise dell for my new puter at home.

Comment 6

[Peter Van der Beken [:peterv]]

Heikki, the change in variable name is intended, we need to create an Expr, not
a PatternExpr. Note that i put a check for expr in XSLTProcessor: 

+                if (!expr) {
+                    // XXX ErrorReport: out of memory
+                    break;
+                }

If we're not able to create the error message, let's just fail silently for now
(and not crash). That would be an extreme case.

The select attribute on apply-templates is very common in XSLT stylesheets, and
anyone making an error in the expression will crash the browser. I'm not sure
how to find sites that have these bad expressions, there's probably not many of
them, but this is going to hurt developers as they'll need to restart their
browser any time they test a stylesheet and they've made a mistake.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

peterv: but there is still this one case where you do not check for out of
memory, please check my earlier comment.

So the crash should mostly affect developers, although I can see that it can
easily trickle to users if developers test in IE only... And it seems like it
really would drive developers berserk when they were developing an XSLT
stylesheet and had to restart compared to reloading the page...

Ok, I'll give it a plus. You are in a hurry to get this onto the branch though,
and if PDT gives its approval the checkin needs to happen during the weekend I
believe.

Comment 8

[Peter Van der Beken [:peterv]]

Heikki, I'm not sure I understand you fully then. If we fail to create the
ErrorFunctionCall, we put null in the expression hash and return it. That's why
I put the check in XSLTProcessor too, for the extreme case where we are not able
to create the ErrorFunctionCall in getExpr.

This code in XSLTProcessor guards against null

+                expr = ps->getExpr(selectAtt);
+                if (!expr) {
+                    // XXX ErrorReport: out of memory
+                    break;
+                }

for when in getExpr, in the following code

+        if (!pExpr) {
+            String err = "Error in parsing XPath pattern: ";
+            err.append(pattern);
+            pExpr = new ErrorFunctionCall(err);
+        }

pExpr should end up being null due to out-of-memory or something like that.

Comment 9

[Peter Van der Beken [:peterv]]

Hmm, i took the wrong snippet. The second snippet was supposed to be

 if ( !expr ) {
-            String err = "invalid expression: ";
+            String err = "Error in parsing XPath expression: ";
             err.append(pattern);
             expr = new ErrorFunctionCall(err);
         }

I could add an assertion or something like that, there is not much else we can
do if we can't create the object to report the error.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

In ProcessorState.cpp you do the new thing but you don't bail out or anything if
you can't allocate the memory. If expr and pExpr are 0, will we crash or handle
that gracefully?

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Is this a regression from Netscape 6.1 time? If so, please add regression keyword.

Comment 12

[Peter Van der Beken [:peterv]]

Heikki: we handle that gracefully by bailing out in XSLTProcessor if we get a 
null pointer back. Still have to check if it is a regression.

Comment 13

[Asa Dotzler [:asa]]

0.9.5 is out the door. bumping TM up by one.

Comment 14

[Peter Van der Beken [:peterv]]

Handing over to sicking, his patch for bug 95779 should fix this too.

Comment 15

[Axel Hecht]

This was fixed by the leak plugs I did. They included a bunch of
if (exprResult && ....
so that crasher got fixed.

Comment 16

[Axel Hecht]

we didn't verify for a long time.
I really checked, so VERIFIED.