The following testcase crashes on mozilla-central revision c2526f6786f0 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion): // jsfunfuzz-generated options('strict_mode'); // Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug-826669.js gczeal(9, 2); var g1 = newGlobal(); var g2 = newGlobal(); var dbg = new Debugger(); dbg.addDebuggee(g1); g1.eval('function f() {}'); dbg.findScripts({}); Backtrace: 0 js-dbg-64-dm-clang-darwin-c2526f6786f0 0x0000000105fec032 js::gc::TenuredCell::writeBarrierPost(void*, js::gc::TenuredCell*, js::gc::TenuredCell*) + 290 (Heap.h:591) 1 js-dbg-64-dm-clang-darwin-c2526f6786f0 0x000000010604189d js::FunctionScope::create(js::ExclusiveContext*, JS::Handle<js::FunctionScope::Data*>, bool, bool, JS::Handle<JSFunction*>, JS::Handle<js::Scope*>) + 557 (Scope.h:208) 2 js-dbg-64-dm-clang-darwin-c2526f6786f0 0x0000000105f61595 js::frontend::BytecodeEmitter::EmitterScope::enterFunction(js::frontend::BytecodeEmitter*, js::frontend::FunctionBox*) + 2101 (RootingAPI.h:792) 3 js-dbg-64-dm-clang-darwin-c2526f6786f0 0x0000000105f83afb js::frontend::BytecodeEmitter::emitFunctionFormalParametersAndBody(js::frontend::ParseNode*) + 379 (BytecodeEmitter.cpp:9026) 4 js-dbg-64-dm-clang-darwin-c2526f6786f0 0x0000000105f6aa5f js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*, js::frontend::BytecodeEmitter::EmitLineNumberNote) + 2143 (BytecodeEmitter.cpp:9425) /snip For detailed crash information, see attachment. Locking s-s because GC is on the stack, to be safe.

This crashes opt builds at js::Scope::environmentChainLength as well.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/40c8129cffbf user: Jon Coppeard date: Tue Dec 06 17:25:02 2016 -1000 summary: Bug 1213977 - Don't reset an ongoing incremental GC if AutoKeepAtoms is set r=sfink Jon, is bug 1213977 a likely regressor?

So this looks the the first instance of a problem that the original AutoKeepAtoms behaviour was hiding. What happens here is that we iterate through a compartment's lazy functions and create new references to them. These may have previously been unreachable. If we are in a incremental GC and have finished marking the roots these will never get marked and be finalized. This leads to use after free. This patch is a spot fix that adds a read barrier here. The ultimate fix is to move this behaviour into the cell iterator so all clients get it automatically.

Comment on attachment 8817329 [details] [diff] [review] bug1322420-lazy-function-barrier Review of attachment 8817329 [details] [diff] [review]: ----------------------------------------------------------------- Ugh. I guess this means that findScripts is engaging in necromancy, which is kind of unfortunate. ::: js/src/jscompartment.cpp @@ +1029,5 @@ > + // reference and trigger the read barrier. > + if (cx->zone()->needsIncrementalBarrier()) > + fun->readBarrier(fun); > + > + // TODO: The above checks should be rolled into the cell iterator. Sorry, but can you file a bug for this and put it here?

jimb, does the devtools debugger still use findScripts? It appears that it can keep things alive that otherwise wouldn't be. Which we can keep working, but I just wanted to make sure we need to.

(In reply to Steve Fink [:sfink] [:s:] from comment #5) > Sorry, but can you file a bug for this and put it here? Sure, I should have done so originally.

Updated patch with review comments.

Comment on attachment 8817968 [details] [diff] [review] bug1322420-lazy-function-barrier v2 [Security approval request comment] How easily could an exploit be constructed based on the patch? Hard, requires incremental GC being triggered at the right time. 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? Everything probably. If not all supported branches, which bug introduced the flaw? I guess this goes all the way back to bug 678037. 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.

(In reply to Steve Fink [:sfink] [:s:] from comment #6) > jimb, does the devtools debugger still use findScripts? It appears that it > can keep things alive that otherwise wouldn't be. Which we can keep working, > but I just wanted to make sure we need to. Unfortunately, yes. I'd like to eventually replace it with a breakpoint API that puts more of the work on SpiderMonkey but does not expose GC behavior, resurrect unreachable objects, or otherwise behave like what the GC considers a sociopath.

Comment on attachment 8817968 [details] [diff] [review] bug1322420-lazy-function-barrier v2 sec-approval=dveditz

JSBugMon: This bug has been automatically verified fixed.

Hi :jonco, Is this worth uplifting to Beta51 & Aurora52?

The testcase didn't cause a crash on versions before bug 1213977 landed, but it's a real problem so we should fix it anyway.

Comment on attachment 8817968 [details] [diff] [review] bug1322420-lazy-function-barrier v2 Approval Request Comment [Feature/Bug causing the regression]: Bug 678037. [User impact if declined]: Possible crashes while debugging. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No. [Why is the change risky/not risky?]: The patch is simple and just adds a read barrier. [String changes made/needed]: None.

Comment on attachment 8817968 [details] [diff] [review] bug1322420-lazy-function-barrier v2 Fix a sec-high. Beta51+ & Aurora52+. Should be in 51 beta 10.

https://hg.mozilla.org/releases/mozilla-beta/rev/b4c638e2f6e5 https://hg.mozilla.org/releases/mozilla-aurora/rev/46fdbb0954b9c34f9795361c234077952293cf88

Comment on attachment 8817968 [details] [diff] [review] bug1322420-lazy-function-barrier v2 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug. User impact if declined: Possible crash / security vulnerability. Fix Landed on Version: 53 Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch: None See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 8817968 [details] [diff] [review] bug1322420-lazy-function-barrier v2 Sec-high, meets the ESR triage bar, should be in ESR45.7

(The commit https://hg.mozilla.org/mozilla-central/rev/4e919ac282a4 that was linked to this bug was actually for bug 1322971.)