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

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla29
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8343752 [details] [diff] [review]
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 :)
Attachment #8343752 - Flags: review?(jimb)
(Assignee)

Updated

4 years ago
Blocks: 936971
(Assignee)

Updated

4 years ago
Duplicate of this bug: 936971

Comment 2

4 years ago
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.
Attachment #8343752 - Flags: review?(jimb)
(Assignee)

Comment 3

4 years ago
Created attachment 8344063 [details] [diff] [review]
js-resolveFun-oom.patch

Patch v2, review comments addressed as discussed on IRC.
Assignee: general → choller
Attachment #8343752 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8344063 - Flags: review?(jimb)

Comment 4

4 years ago
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> *.
Attachment #8344063 - Flags: review?(jimb)
(Assignee)

Comment 5

4 years ago
Created attachment 8344083 [details] [diff] [review]
js-resolveFun-oom.patch

Is this how you suggested it? :)
Attachment #8344063 - Attachment is obsolete: true
Attachment #8344083 - Flags: review?(jimb)

Comment 6

4 years ago
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.
Attachment #8344083 - Flags: review?(jimb) → review+
(Assignee)

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/79ae2a4a2e1e
https://hg.mozilla.org/mozilla-central/rev/79ae2a4a2e1e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.