Closed Bug 1312525 Opened 3 years ago Closed 3 years ago
Assertion failure: [barrier verifier] Unmarked edge: lazy
Script, at js/src/gc/Verifier .cpp:311
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.
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?
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...?
It's unlikely to be correct. Perhaps it becomes intermittent as one bisects back...
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.
Assignee: nobody → jcoppeard
Attachment #8805556 - Flags: review?(sphink)
Attachment #8805556 - Flags: review?(sphink) → review+
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.
Attachment #8805556 - Flags: sec-approval?
As 50 is unaffected, I'll give sec-approval+ now for trunk. We'll want a patch for 51 made and nominated as well.
Attachment #8805556 - Flags: sec-approval? → sec-approval+
JSBugMon: This bug has been automatically verified fixed.
Hi :jonco, Since this bug also affects 51, do you think it's worth uplifting to 51?
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.
Attachment #8805556 - Flags: approval-mozilla-aurora?
Comment on attachment 8805556 [details] [diff] [review] bug1312525-lazy-script-barrier Fix sec-high. Take it in 51 aurora.
Attachment #8805556 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx51
You need to log in before you can comment on or make changes to this bug.