174093 - Make ExprParser out-of-memory failsafe

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The summary says it all...

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch v1

Comment 2

[Axel Hecht]

Comment on attachment 102664 [details] [diff] [review]
patch v1

>@@ -404,12 +428,20 @@
>             return 0;
>             break;
>     }
>-    if (!expr)
>+    if (!expr) {
>+        // XXX ErrorReport: out of memory
>+        delete expr;

it's null already, no need to delete it

>@@ -531,11 +563,16 @@
>         rv = aContext->resolveFunctionCall(lName, namespaceID, fnCall);
>         TX_IF_RELEASE_ATOM(prefix);
>         TX_IF_RELEASE_ATOM(lName);
>-        if (NS_FAILED(rv) && rv != NS_ERROR_NOT_IMPLEMENTED) {
>-            // XXX report error unknown function call
>+        if (NS_FAILED(rv)) {
>+            // XXX report error unknown/not-implemented function call
>             return 0;
>         }
>     }
>+
>+    if (!fnCall) {
>+        // XXX ErrorReport: out of memory
>+        return 0;
>+    }

We should definitly return an error rv if we have an out of mem.
Don't catch that error twice. You could ASSERT, though.
Do you really want to change the current behaviour of having
an error expr in the result for unknown functions?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

returning an rv might be good to do but is IMHO out of scope for this bug. We
currently return 0 if something has failed. Don't really see the point in
asserting when we run out of memory, unless we think there is something wrong
with our code if we run out of memory in this place?

Yes, I don't see a reason to parse an expression that can't be executed. With
proper errorhandling outside the parser we should abort the transformation when
we hit an unknown function IMHO.

I'll attach a patch with first comment fixed

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch v2

fixes the problem right after line 404

Comment 5

[Axel Hecht]

> returning an rv might be good to do but is IMHO out of scope for this bug. We
> currently return 0 if something has failed. Don't really see the point in
> asserting when we run out of memory, unless we think there is something wrong
> with our code if we run out of memory in this place?

IMHO, ProcessorState should check for out-of-memory
 
> Yes, I don't see a reason to parse an expression that can't be executed. With
> proper errorhandling outside the parser we should abort the transformation when
> we hit an unknown function IMHO.

NS_ERROR_NOT_IMPLEMENTED is returned for functions that are part of the standard,
but not implemented. So they are not really an error in the stylesheet, and 
bailing out completely on this seems like a bad idea to me

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> IMHO, ProcessorState should check for out-of-memory

why? how? and do what if we are oom?

I do see a point on the unparsed-entity-uri function, though i don't really 
care either way. But if we don't want to bail for that specific case it would 
be better to implement that function now (with class and all) and have it 
always return some errormessage. I'll remove that part of the patch for now 
though.

Comment 7

[Axel Hecht]

http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/source/xslt/ProcessorState.cpp#1098
ProcessorState::resolveFunctionCall should return NS_ERROR_OUT_OF_MEMORY (sp?)
instead of assuming that 
  aFunction = new DocumentFunctionCall(this, aElem);
and friends succeeded.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ah, good catch

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: fixes pikes comments

Comment 10

[Axel Hecht]

Comment on attachment 107263 [details] [diff] [review]
fixes pikes comments

>Index: source/xpath/ExprParser.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xpath/ExprParser.cpp,v
>retrieving revision 1.26
>diff -u -r1.26 ExprParser.cpp
>--- source/xpath/ExprParser.cpp	22 Aug 2002 12:17:26 -0000	1.26
>+++ source/xpath/ExprParser.cpp	24 Nov 2002 05:02:37 -0000


>@@ -531,11 +562,30 @@
>         rv = aContext->resolveFunctionCall(lName, namespaceID, fnCall);
>         TX_IF_RELEASE_ATOM(prefix);
>         TX_IF_RELEASE_ATOM(lName);
>-        if (NS_FAILED(rv) && rv != NS_ERROR_NOT_IMPLEMENTED) {
>-            // XXX report error unknown function call
>+
>+        // XXX this should be removed once we don't return
>+        // NS_ERROR_NOT_IMPLEMENTED for unparsed-entity-uri(). As should the
>+        // code in parseParameters that deals with fnCall = 0.
>+        // Bug 65981
>+        if (rv == NS_ERROR_NOT_IMPLEMENTED) {
>+            NS_ASSERTION(!fnCall, "Now is it implemented or not?");
>+            if (!parseParameters(0, lexer, aContext)) {
>+                return 0;
>+            }
>+            String err(tok->value);
>+            err.append(" not implemented.");
>+            return new StringExpr(err);
>+        }
>+
>+        if (NS_FAILED(rv)) {
>             return 0;
>         }
>     }
>+
>+    if (!fnCall) {
>+        // XXX ErrorReport: out of memory
>+        return 0;
>+    }

already caught, rv's should be correct. If you're scared, assert
>     
>     //-- handle parametes
>     if (!parseParameters(fnCall, lexer, aContext)) {

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> already caught, rv's should be correct. If you're scared, assert

It's for checking the codepaths where we don't call resolveFunctionCall, i.e.
all the:

     if (XPathNames::BOOLEAN_FN.isEqual(tok->value)) {
         fnCall = new BooleanFunctionCall(BooleanFunctionCall::TX_BOOLEAN);

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

requesting approval. This patch is very lowrisk as it simply adds a few
out-of-memory checks and doesn't change any logic unless we run out of memory.

Comment 13

[Axel Hecht]

ah, ok. Could you add a comment, as with my typical editor window size, it's
a bit out of context. It's at the right place, but not appearantly. Just a 
// Failed to create XPath function call above.
would ease my mind.

Comment 14

[Asa Dotzler [:asa]]

blocking1.3a is a nomination. set the ? to nominate the bug as something for
[e-mail] to consider as blocking 1.3a. Only [e-mail] are
allowed to set the + and - on that flag. To request approval from drivers set
the "approval1.3a ?" flag on a reviewed and super-reviewed patch. Thanks.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 107263 [details] [diff] [review]
fixes pikes comments

eek, sorry, my misstake

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 107263 [details] [diff] [review]
fixes pikes comments

a=asa for checkin to 1.3a

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in

Comment 18

[Axel Hecht]

mass verifying