947233 - Assertion failure: !cx->isExceptionPending(), at ../jsapi.cpp:2526 due to OOM in NameResolver

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: js-resolveFun-oom.patch

Hitting this assertion from time to time while fuzzing, with no good tests available. I've analyzed the problem and it seems that in [@ resolveFun] can fail due to OOM (and will return NULL in that case), but also returns NULL in other non-failure cases.

The attached patch

* refactors [@ resolveFun] to return bool to indicate success instead of returning the pointer directly. I've converted all returns to true except the one failing to due OOM. One or the other returns could also be hit by an OOM path but I'm not sure about that, so I'm leaving them alone for now.

* refactors [@ resolve] to return bool to propagate the issue, as jimb requested. 
* passes all jit-tests
* fixes the assertion (confirmed on a fuzzing box).

Feedback welcome :)

Comment 2

[Jim Blandy :jimb]

Comment on attachment 8343752 [details] [diff] [review]
js-resolveFun-oom.patch

Review of attachment 8343752 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is right. resolveFun should return true on success, false on OOM. You've got it returning true on OOM, in many cases; I've pointed out a few below, but there are others. Also, when we do get an OOM, we probably don't need to bother assigning anything to *retAtom.

The changes to 'resolve' all look right, though.

::: js/src/frontend/NameFunctions.cpp
@@ +189,4 @@
>              if (!buf.append(prefix) ||
>                  !buf.append("/") ||
>                  !buf.append(fun->displayAtom()))
> +                return true;

Here's one that seems to me it should be 'return false'.

@@ +196,5 @@
>  
>          /* If a prefix is specified, then it is a form of namespace */
> +        if (prefix != nullptr && (!buf.append(prefix) || !buf.append("/"))) {
> +            *retAtom = nullptr;
> +            return true;

Here's another one which looks like an OOM path to me.

Comment 3

[Christian Holler (:decoder)]

Attachment: js-resolveFun-oom.patch

Patch v2, review comments addressed as discussed on IRC.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8344063 [details] [diff] [review]
js-resolveFun-oom.patch

Review of attachment 8344063 [details] [diff] [review]:
-----------------------------------------------------------------

Looks much better. Still one fix needed. Also a suggestion.

::: js/src/frontend/NameFunctions.cpp
@@ +172,5 @@
>       * Resolve the name of a function. If the function already has a name
>       * listed, then it is skipped. Otherwise an intelligent name is guessed to
>       * assign to the function's displayAtom field
>       */
> +    bool resolveFun(ParseNode *pn, HandleAtom prefix, JSAtom** retAtom) {

Could we use a MutableHandleAtom for 'retAtom' here, with the appropriate changes to the caller?

@@ +245,5 @@
>           * other namespace are rather considered as "contributing" to the outer
>           * function, so give them a contribution symbol here.
>           */
> +        if ((!buf.empty() && *(buf.end() - 1) == '/' && !buf.append("<")) || buf.empty())
> +            return false;

You merged two 'return nullptr' statements here. I think the first was an OOM return, so 'return false' is right.

But the second, where buf.empty() is true, was a "we have no name" return - which should be a 'return true' with *retAtom == nullptr.

@@ +281,5 @@
> +            JSAtom* resolvedFun = nullptr;
> +            if (!resolveFun(cur, prefix, &resolvedFun))
> +                return false;
> +
> +            RootedAtom prefix2(cx, resolvedFun);

As suggested above: let's construct the RootedAtom first, and then pass it to resolvedFun as a MutableHandleAtom. Should be as simple as passing '&prefix2'; MutableHandle<T> can be constructed from a Rooted<T> *.

Comment 5

[Christian Holler (:decoder)]

Attachment: js-resolveFun-oom.patch

Is this how you suggested it? :)

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8344083 [details] [diff] [review]
js-resolveFun-oom.patch

Review of attachment 8344083 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Just one thing that needs to be fixed, and this can land.

::: js/src/frontend/NameFunctions.cpp
@@ +244,5 @@
>           * functions which are "genuinely anonymous" but are contained in some
>           * other namespace are rather considered as "contributing" to the outer
>           * function, so give them a contribution symbol here.
>           */
> +        if ((!buf.empty() && *(buf.end() - 1) == '/' && !buf.append("<")) || buf.empty())

Don't forget to take out that '|| buf.empty()'! :)

@@ +282,3 @@
>  
>          if (cur->isKind(PNK_FUNCTION) && cur->isArity(PN_CODE)) {
> +            RootedAtom prefix2(cx, nullptr);

If you just leave off the ', nullptr', that's what the constructor will use by default.

Comment 7

[Christian Holler (:decoder)]

Thanks :) Made the changes and it's green on try. Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/79ae2a4a2e1e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79ae2a4a2e1e