680755 - Missing goto out in CompileUCFunctionForPrincipalsCommon

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Since the bug 679879 CompileUCFunctionForPrincipalsCommon from jsapi.cpp contains http://hg.mozilla.org/mozilla-central/annotate/6dc468c41136/js/src/jsapi.cpp#l4724 :

        EmptyShape *emptyCallShape = EmptyShape::getEmptyCallShape(cx);
        if (!emptyCallShape)
            fun = NULL;
        AutoShapeRooter shapeRoot(cx, emptyCallShape);

        AutoObjectRooter tvr(cx, fun);

That is, there is a missing goto out here. This bug leads to a NULL pointer deref in case EmptyShape::getEmptyCallShape fails to allocate the shape.

Comment 1

[Igor Bukanov]

Attachment: v 0.1

The patch adds an auto class to do the last-frame-checks and uses it in CompileUCFunctionForPrincipalsCommon to replace replace LAST_FRAME_CHECKS macro. This way on errors the code can immediately return null.

If the proposed usage looks OK, I will replace all LAST_FRAME_CHECKS with the new class.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 554714 [details] [diff] [review]
v 0.1

This is a fine idea. I'll be happy to review, but the patch has a bug: LAST_FRAME_CHECKS only reports an exception if no JS code is running on cx (hence the words "last frame" in the name). This is by design. It means that JSNatives and other callbacks can call public JSAPI functions like so:

    if (!JS_CallFunctionName(cx, obj, "foo", 0, NULL, &v))
        return false;

and any exceptions thrown from obj.foo() will be propagated to the caller, not squelched or converted to uncatchable errors. (If we don't have a test for this, consider writing a jsapi-test.)

I think the success() method is unnecessary. It's harmless to check for an exception on success.

Comment 3

[Igor Bukanov]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> This is a fine idea. I'll be happy to review, but the patch has a bug:
> LAST_FRAME_CHECKS only reports an exception if no JS code is running on cx
> (hence the words "last frame" in the name). 

Thanks for catching this, the destructor were supposed to look like:

if (cx && JS_IsRunning(cx) && !cx->hasRunOption(JSOPTION_DONT_REPORT_UNCAUGHT))
    js_ReportUncaughtException(cx);

> I think the success() method is unnecessary. It's harmless to check for an
> exception on success.

Do you suggest to check for cx->throwing instead? That would indeed remove the need for the success method.

Comment 4

[Igor Bukanov]

(In reply to Igor Bukanov from comment #3)
> (In reply to Jason Orendorff [:jorendorff] from comment #2)
> > I think the success() method is unnecessary. It's harmless to check for an
> > exception on success.
> 
> Do you suggest to check for cx->throwing instead? That would indeed remove
> the need for the success method.

The drawback of checking for cx->throwing is an extra branch per API call. With the explicit success() the compiler can optimize away the exception check at places where the code checks for errors in any case. But I presume that would not affect the benchmarks...

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Igor Bukanov from comment #3)
> Do you suggest to check for cx->throwing instead? That would indeed remove
> the need for the success method.

Yes.

(In reply to Igor Bukanov from comment #4)
> The drawback of checking for cx->throwing is an extra branch per API call.
> With the explicit success() the compiler can optimize away the exception
> check at places where the code checks for errors in any case.

I actually find this fairly convincing. Keep it if you prefer.

Comment 6

[Igor Bukanov]

Attachment: v1

The new version removes the success method and checks for cx->throwing instead. In most places where the last frame checks are used we do not check for the error explicitly and just return the result of the internal api call directly.  So the issues from the comment 4 is not common to justify extra code complexity.

The patch also removes unnecessary anchoring and rooting around js_NewFunction in couple of places via rearranging the sequence of calls so compileFunctionBody immediately follows js_NewFunction.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 555548 [details] [diff] [review]
v1

Nice cleanup.

Comment 8

[Igor Bukanov]

http://hg.mozilla.org/integration/mozilla-inbound/rev/62fd8154f8de

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/62fd8154f8de