1459932 - Crash in NameResolver::resolve

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-3d6d00d9-8526-4d8c-86e5-c97e70180505.
=============================================================

Seen while looking at Android crash stats - this crash appears in the Fennec 60 rc: https://bit.ly/2HWL8Bb. This crash is also present in 59. 67 crashes/31 installs so far on RC - on 59.0.2 we have 241 crashes/112 installs in a 7 day period.

Correlation shows (90.05% in signature vs 01.16% overall) Module "linker64" = true, although not sure if that means anything.

APIs from 23-27 are affected. 

Top 1 frames of crashing thread:

0 libxul.so NameResolver::resolve js/public/RootingAPI.h:942

=============================================================

Comment 1

[Mike Taylor [:miketaylr]]

2 website in the comments. Steve, can you take a look?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

all wildptr crashes

Comment 3

[Jan de Mooij [:jandem]]

Putting this in our crash triage queue.

Comment 4

[Steven DeTar [:sdetar]]

Removing my needinfo since Jan put this bug in our crash triage queue where it will be looked into in more detail soon.

Comment 5

[Ted Campbell [:tcampbell]]

When looking at other variations of the same signature, I see other platforms crashing too.

Comment 6

[Ted Campbell [:tcampbell]]

These crashes seem to have a number of resolve calls on the stack (expected) before either |cur| is garbage or the RootedAtom::stack pointer gets stomped.

The actual crash location varies, but the second last crash is typically [1] and this is the last time |cur| is current so I'd paint the bullseye here. It isn't clear from the crashes which of these cases is failing here.

[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/106b1066bb3fb6a4821a8da52cc913846149928a/js/src/frontend/NameFunctions.cpp#l444

Comment 7

[Ted Campbell [:tcampbell]]

Jason, these crashes seem to stem from [1]. Can you add a release assert or otherwise track down what's going off the rails?

[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/106b1066bb3fb6a4821a8da52cc913846149928a/js/src/frontend/NameFunctions.cpp#l444

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

3 months with a NI for a release assert to be added...  bumping

Comment 9

[Steven DeTar [:sdetar]]

Jason, any updates on adding the release assert Ted suggested?

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 9005507 [details] [diff] [review]
Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

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

Fuzzy logic here, but I like it.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

This adds one release-mode assertion in NameResolver::resolve(), but most of
the work is plumbing for a new debug-only assertion (for the fuzzers).

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

This adds one release-mode assertion in NameResolver::resolve(), but most of
the work is plumbing for a new debug-only assertion (for the fuzzers).

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 9005629 [details] [diff] [review]
Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

Carrying Jeff's review forward.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

    The patch does not fix a bug, it just adds assertions. It is unlikely
    an exploit could be constructed based on this.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

    They point out a place in the code where we do have crashes.
    But it isn't clear (even to us) what the bug is or how to trigger it.

Which older supported branches are affected by this flaw?

    All.

How likely is this patch to cause regressions; how much testing does it need?

    Normal testing will suffice.

    Most likely regression: the new MOZ_RELEASE_ASSERT in this patch is
    not sound, hits way more than expected in practice, and has to be backed
    out. This is not likely.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 9005629 [details] [diff] [review]
Add assertions that all parse nodes visited in NameFunctions are in the parser's allocator

Clearing sec-approval. The DEBUG-only assertion calling alloc.contains() is too slow. I need to fix that and get review again.

Comment 17

[Jason Orendorff [:jorendorff]]

Bug 1489572 might make LifoAlloc::contains fast enough that this assertion can land. If that's not enough, or if the patches there can't land, I can add DEBUG-only bookkeeping to LifoAlloc. nbp suggested a SplayTree. But post-1489572, if there are at most ~50 regions per LifoAlloc, a SplayTree would be overkill; we'd want a vector of pointers, for simplicity and locality.

Anyway -- I'm going to give this a week and see if bug 1489572 magically makes this fast enough.

Again, the assertion doesn't fix this bug; it just gives us a chance of getting lucky and finding it.

Comment 18

[Jason Orendorff [:jorendorff]]

Bug 1489572 is looking good.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Seems unlikely to make it to 64. Jason, what do you think?

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Anything further to do here or should we call this stalled? (I'll follow up in email.)

Comment 21

[Jason Orendorff [:jorendorff]]

Bug 1529607 partly rewrote this code.

If crashes still happen in NameFunction.cpp, it's the same bug, but the signature will change since that resolve() method is gone.

Comment 22

[Jason Orendorff [:jorendorff]]

This seems to be fixed in Firefox 67 by the rewrite in bug 1529607.