1312525 - Assertion failure: [barrier verifier] Unmarked edge: lazyScript, at js/src/gc/Verifier.cpp:311

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 215f96861176 (build with --enable-debug, run with --fuzzing-safe --no-threads --ion-eager):

See attachment.

Backtrace:

0   js-dbg-64-clang-darwin-215f96861176	0x0000000100b5969a js::gc::GCRuntime::endVerifyPreBarriers() + 1114 (Verifier.cpp:312)
1   js-dbg-64-clang-darwin-215f96861176	0x0000000100b5981f js::gc::MaybeVerifyBarriers(JSContext*, bool) + 79 (Verifier.cpp:406)
2   js-dbg-64-clang-darwin-215f96861176	0x000000010089306d Interpret(JSContext*, js::RunState&) + 6317 (Interpreter.cpp:4174)
3   js-dbg-64-clang-darwin-215f96861176	0x00000001008915eb js::RunScript(JSContext*, js::RunState&) + 443 (Interpreter.cpp:404)
4   js-dbg-64-clang-darwin-215f96861176	0x00000001008a265f js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 511 (Interpreter.cpp:476)
5   js-dbg-64-clang-darwin-215f96861176	0x000000010089a177 Interpret(JSContext*, js::RunState&) + 35255 (Interpreter.cpp:2922)
6   js-dbg-64-clang-darwin-215f96861176	0x00000001008915eb js::RunScript(JSContext*, js::RunState&) + 443 (Interpreter.cpp:404)
/snip

For detailed crash information, see attachment.

Setting s-s by default because this involves barriers and GC is on the stack.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Testcase

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6e6438f5d89f
user:        André Bargull
date:        Thu Oct 06 23:38:16 2016 -0700
summary:     Bug 1303788 - Add support for trailing comma in argument and parameter lists (ES2017). r=arai

Not sure if this is correct. The testcase is also really hard to further reduce. Jon, what do you think?

Comment 4

[André Bargull [:anba]]

FWIW I've undone all changes from my commit one by one and the crash still happened. So I'm not sure the bisect is correct...?

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

It's unlikely to be correct. Perhaps it becomes intermittent as one bisects back...

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1312525-lazy-script-barrier

There's a comment in JSFunction::setUnlazifiedScript that implies we don't need a barrier here, but it doesn't cover all callers.  Always barrier here.

I also added some extra output to the pre-barrier verifier to make it easier to track these things down.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8805556 [details] [diff] [review]
bug1312525-lazy-script-barrier

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

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

Which older supported branches are affected by this flaw? 51.

If not all supported branches, which bug introduced the flaw? It looks like bug 1263355 caused this by adding a call to JSFunction::setUnlazifiedScript that didn't have a barrier.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial.

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

Comment 8

[Gerry Chang [:gchang]]

Track 51+/52+ as sec-high.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

As 50 is unaffected, I'll give sec-approval+ now for trunk. We'll want a patch for 51 made and nominated as well.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aacc86dbfeea0cbcf4e46bef3d531e52aa6b0165

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aacc86dbfeea

Comment 12

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[Gerry Chang [:gchang]]

Hi :jonco, 
Since this bug also affects 51, do you think it's worth uplifting to 51?

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 8805556 [details] [diff] [review]
bug1312525-lazy-script-barrier

Approval Request Comment
[Feature/regressing bug #]: Bug 1263355
[User impact if declined]: Possbile crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since 3rd November.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8805556 [details] [diff] [review]
bug1312525-lazy-script-barrier

Fix sec-high. Take it in 51 aurora.

Comment 16

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a8ed7ba82adecdd095ae3d7d99bcf672ef7e976

Comment 17

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx51