Closed Bug 1312525 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 + verified
firefox52 + verified

People

(Reporter: gkw, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Attachments

(3 files)

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.
Attached file Testcase
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?
Flags: needinfo?(jcoppeard)
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...
Keywords: sec-high
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
Flags: needinfo?(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?
Blocks: 1263355
Track 51+/52+ as sec-high.
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+
https://hg.mozilla.org/mozilla-central/rev/aacc86dbfeea
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(jcoppeard)
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
Group: javascript-core-security → core-security-release
Group: core-security-release
Keywords: regression
You need to log in before you can comment on or make changes to this bug.