The following testcase crashes on mozilla-central revision 2e3e89c9c68c (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off): var lfLogBuffer = ` function evalWithCache(code, ctx) { ctx = Object.create(ctx, {}); code = code instanceof Object ? code : cacheEntry(code); var incremental = ctx.incremental || false; ctx.global = newGlobal({ cloneSingletons: !incremental }); ctx_save = Object.create(ctx, {saveBytecode: { value: true } }); var res1 = evaluate(code, ctx_save); var res3 = evaluate(code, Object.create(ctx, {loadBytecode: { value: true } })); } var test = "new class extends class { } { constructor() { super(); } }()"; evalWithCache(test, { assertEqBytecode : true }); `; loadFile(lfLogBuffer); function loadFile(lfVarx, lfForceModule = false) { try { oomTest(new Function(lfVarx)); } catch (lfVare) {} } Backtrace: received signal SIGSEGV, Segmentation fault. js::CheckTracedThing<JSString> (trc=trc@entry=0x7fffffffafa8, thing=0xfffe4ccccccccccc) at js/src/gc/Marking.cpp:210 #0 js::CheckTracedThing<JSString> (trc=trc@entry=0x7fffffffafa8, thing=0xfffe4ccccccccccc) at js/src/gc/Marking.cpp:210 #1 0x0000555555893958 in DoCallback<JSString*> (trc=0x7fffffffafa0, thingp=0x7fffffffaf30, name=0x555556501d80 "scope name") at js/src/gc/Tracer.cpp:49 #2 0x00005555561b78aa in js::TraceManuallyBarrieredEdge<JSAtom*> (name=0x555556501d80 "scope name", thingp=0x7fffffffaf30, trc=0x7fffffffafa8) at js/src/gc/Tracer.h:187 #3 TraceNullableBindingNames (length=<optimized out>, names=0x7ffff4ab3e30, trc=0x7fffffffafa8) at js/src/gc/Marking.cpp:1235 #4 js::FunctionScope::Data::trace (this=this@entry=0x7ffff4ab3e00, trc=trc@entry=0x7fffffffafa8) at js/src/gc/Marking.cpp:1260 #5 0x0000555555f02d62 in js::GCManagedDeletePolicy<js::FunctionScope::Data>::operator() (this=<optimized out>, constPtr=0x7ffff4ab3e00) at js/src/gc/DeletePolicy.h:80 #6 DeleteScopeData<js::FunctionScope::Data> (data=0x7ffff4ab3e00) at js/src/vm/Scope.cpp:266 #7 mozilla::Result<mozilla::Ok, JS::TranscodeResult> js::Scope::XDRSizedBindingNames<js::FunctionScope, (js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::Handle<js::FunctionScope*>, JS::MutableHandle<js::FunctionScope::Data*>)::{lambda()#1}::operator()() const (__closure=0x7fffffffaf90) at js/src/vm/Scope.cpp:296 #8 mozilla::ScopeExit<mozilla::Result<mozilla::Ok, JS::TranscodeResult> js::Scope::XDRSizedBindingNames<js::FunctionScope, (js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::Handle<js::FunctionScope*>, JS::MutableHandle<js::FunctionScope::Data*>)::{lambda()#1}>::~ScopeExit() (this=0x7fffffffaf90, __in_chrg=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/debug/dist/include/mozilla/ScopeExit.h:113 #9 js::Scope::XDRSizedBindingNames<js::FunctionScope, (js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, scope=..., data=...) at js/src/vm/Scope.cpp:299 #10 0x0000555555f0b7db in js::FunctionScope::XDR<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, fun=..., fun@entry=..., enclosing=..., enclosing@entry=..., scope=...) at js/src/vm/Scope.cpp:782 #11 0x0000555555e6fa20 in js::XDRScript<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, scriptEnclosingScope=scriptEnclosingScope@entry=..., sourceObjectArg=..., fun=..., fun@entry=..., scriptp=...) at js/src/vm/JSScript.cpp:780 #12 0x0000555555e70b37 in js::XDRInterpretedFunction<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, enclosingScope=..., enclosingScope@entry=..., sourceObject=..., sourceObject@entry=..., objp=..., objp@entry=...) at js/src/vm/JSFunction.cpp:705 #13 0x0000555555e6fc74 in js::XDRScript<(js::XDRMode)1> (xdr=xdr@entry=0x7fffffffb860, scriptEnclosingScope=..., scriptEnclosingScope@entry=..., sourceObjectArg=..., fun=fun@entry=..., scriptp=...) at js/src/vm/JSScript.cpp:900 #14 0x0000555555ffcaf4 in js::XDRState<(js::XDRMode)1>::codeScript (this=this@entry=0x7fffffffb860, scriptp=...) at js/src/vm/Xdr.cpp:200 #15 0x0000555555c69a0f in JS::DecodeScript (cx=<optimized out>, buffer=..., scriptp=..., cursorIndex=<optimized out>) at js/src/jsapi.cpp:6950 #16 0x0000555555680848 in Evaluate (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:2103 #17 0x00002db8d48a453b in ?? () [...] #20 0x0000000000000000 in ?? () rax 0x0 0 rbx 0xfffe4ccccccccccc -478507460408116 rcx 0x0 0 rdx 0x555556501d80 93825008672128 rsi 0xfffe4ccccccccccc -478507460408116 rdi 0x7fffffffafa8 140737488334760 rbp 0x7fffffffaed0 140737488334544 rsp 0x7fffffffaeb0 140737488334512 r8 0x7fffffffab70 140737488333680 r9 0x6cb 1739 r10 0x7fffffffac50 140737488333904 r11 0x7ffff6cb9f90 140737333927824 r12 0x7fffffffafa8 140737488334760 r13 0x555556501d80 93825008672128 r14 0x7fffffffaf30 140737488334640 r15 0x7ffff5f16000 140737319624704 rip 0x5555561e0e39 <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+41> => 0x5555561e0e39 <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+41>: testb $0x1,(%rsi) 0x5555561e0e3c <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+44>: jne 0x5555561e0fd8 <js::CheckTracedThing<JSString>(JSTracer*, JSString*)+456> Marking s-s because the register we read contains a garbage address.

JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20151013053056" and the hash "8d9c20c241be7d7b3cfa90a3368a77db42172781". The "bad" changeset has the timestamp "20151013054956" and the hash "d80f9d6921f8209ef01aa730be9a97ab727704d1". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d9c20c241be7d7b3cfa90a3368a77db42172781&tochange=d80f9d6921f8209ef01aa730be9a97ab727704d1

We're poisoning the TrailingNamesArray, but this data is required to be valid in case we have to delete the scope data objects outside GC. This happens because some fields (e.g. FunctionScope::Data::canonicalFunction) may have store buffer entries that point to them, and we have to remove these if the object is deleted at a time the store buffer is not known to be empty. We do this by tracing the object when it is deleted.

Patch to construct the BindingNames when the TrailingNamesArray is created rather than leave them as uninitialized memory.

So the posted patch works, but. It adds a double-write to stuff that might matter for perf. It gets rid of some poisoning that might detect misuses faster. And it doesn't fix a similar problem with WasmInstanceScope::Data. I think we can do better. So, um, I went and wrote several patches to fix this without double-writing. :-) I also wrote a nifty little template function algorithm for filling trailing names in fresh Data instances in the parser -- that only adds five lines (and of course templates), but gets rid of some messy index-tracking/cursor-handling code in the process. I think it's well worth it for clarity at point of use, but you tell me. :-) It'll be the last patch I post here. Anyway. This is just a first patch that makes one more place use placement-new, that already should have.

This makes the length field in the XDR'd stuff always reflect exactly the names added so far, so if a trace happens, we'll trace exactly the good names, no more, no less. This fixes this bug as originally reported.

It'd be nice if someone wrote a test that would trigger the same problem with this code, as comment 0 found for the XDR stuff. I definitely don't have time to ramp up on wasm now to do so myself.

This is IMO surprisingly elegant for C++ template recursion. And I think the concision of // The ordering here is important. See comments in GlobalScope. InitializeBindingData(bindings, numBindings, vars, &GlobalScope::Data::letStart, lets, &GlobalScope::Data::constStart, consts); is far better than // The ordering here is important. See comments in GlobalScope. BindingName* start = bindings->trailingNames.start(); BindingName* cursor = start; cursor = FreshlyInitializeBindings(cursor, vars); bindings->letStart = cursor - start; cursor = FreshlyInitializeBindings(cursor, lets); bindings->constStart = cursor - start; Unused << FreshlyInitializeBindings(cursor, consts); bindings->length = numBindings; where we're manually slinging around pointers, we have MOZ_MUST_USE on FreshlyInitializeBindings so we don't forget to use an advanced cursor every time, we gotta Unused the final call where the at-end cursor is never needed, and there's a ton of noise for every pair of member-field/copied-vector pairing being set. But strictly speaking, this patch is not necessary to fix this bug and could be rejected if you were a hater. :-) Assuming all these patches are go, I probably would fold them all together into one for landing, as obfuscatory tactic and to not give away the game too much. It probably would be reasonably non-obvious exactly where the problem was in all that.

Comment on attachment 9013323 [details] [diff] [review] bug1494752-scope-oom I'm fine with your approach. I'm not sure how much difference the double-write would make to performance, but it is better not to lose the poisoning.

Comment on attachment 9016155 [details] [diff] [review] Ensure Data::length always reflects the trailing names that have actually been initialiezd during XDR decoding Review of attachment 9016155 [details] [diff] [review]: ----------------------------------------------------------------- Please do land the test case at some point too.

Comment on attachment 9016158 [details] [diff] [review] Infallibly initialize the various |Data::trailingNames| bindings using a variadic template function to abstract logic -- and that, because it does only infallible operations, clearly need update |Data::length| only once at its end Review of attachment 9016158 [details] [diff] [review]: ----------------------------------------------------------------- This is neat. Thanks for the explanatory comments on InitializeBindingData.

Waldo tells me this goes back to bug 1263355.

[Security Approval Request] How easily could an exploit be constructed based on the patch?: Depends how easily you can trigger a GC at just the wrong time, then get memory ostensibly containing BindingNames to contain prior data of the right sort. Probably requires some memory allocator awareness. But it seems doable with good chops, and maybe really-doable if Metasploit sorts of things mean you don't have to deeply understand allocator internals to craft an attack. The patch comments don't squarely point out the issue, but it's possible a not-deeply-educated-in-this-code person could pick out the salient lines -- places where we increment |data->length| a bunch of times, versus setting it just once -- and then craft an attack from that. It's hard to say, given I understand the issue and am trying to place myself in the shoes of someone who does not. 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 that includes the frontend rewrite...so probably everything If not all supported branches, which bug introduced the flaw?: Bug 1263355 Do you have backports for the affected branches?: No If not, how different, hard to create, and risky will they be?: I have a beta backport. It wasn't too bad to make, as long as I generated a rollup patch with little enough context to apply to the old tree, and I regressed a few places in the beta-tree to Gecko bracing style. There's a fair chance the patch would also apply to esr60, but I'm still cloning an esr60 to find out just how easily the patch would apply. How likely is this patch to cause regressions; how much testing does it need?: It's generally fairly straightforward, IMO. The trace-functions are all quite clear about tracing up to |data->length| (and have been since bug 1263355 landed), and this patchwork fairly clearly adds trailing names exactly in sync with length-updates, when intervening operations could be fallible. I don't foresee regressions. But this is a place where finickiness applies, so I would probably say we get a moderate amount of testing -- say, land a week before branch drop-dead date or so, ideally. But I could take landing later in the cycle if it came to it.

No particular interesting changes were required to make this backport, and a readthrough of the patch indicates all the things that need to change, were changed as needed, and correctly so. So this is good for whenever after the trunk patch lands, more or less.

We don't have the TrailingNamesArray helper on ESR60, but what we do have is pretty similar, and it wasn't too bad to make the necessary adjustments. The Parser.cpp changes required similar sorts of not-difficult dealing, because ESR60 uses PodCopy to fill in binding data rather than playing nice with C++, but it was pretty good. Of note: I carefully wrote the InitializeBindingData calls by looking at the existing ESR60 code, because some of the vectors/saved-indexes stuff in 60 is different from what exists on trunk -- so we shouldn't have mistakes due to brain-off backporting there.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96e3d756e9d1d6e52ac9b8a9635ce825b7fe69d0 for a one-platform try-run of the trunk patch, with an obfuscatory commit message that hopefully doesn't clue anyone watching in as to it being a sec issue since bug 1136954 isn't fixt yet. :-|

Comment on attachment 9016483 [details] [diff] [review] Rollup patch for trunk > I would probably say we get a moderate amount of testing -- say, land a > week before branch drop-dead date or so, ideally. We are basically already there if you were thinking this is safe enough to uplift to Fx63. RCs will be built next week so there's no "nightly bake time" or even "beta test" time if you land now (there will be a few days of beta folks getting the RC before the actual ship date). So is this patch safe enough for that aggressive schedule or should we punt until Firefox 64?

Mm. I want to say this is safe. But also this is somewhat similar in nature to bug 1462939 -- which I patched, which had issues of doing it wrongly in the first version, then required a few followup fix rounds after. Which means there's probably greater risk of issue from this being finicky, but also less risk because I have practice getting it right. :-) So maybe it's a wash, but having to weigh odds both directions ups the riskiness. Let's play it safe and not rush it into the last RC. Let me know when I should land this *after* the next few days, so there's a little time for issues to arise before it goes into betas or RCs.

Marking as wontfix for 63 as per the risk analysis in comment #17.

:jesup observed a number of scary crashes in [1]. They seem to point out that js::Scope::XDRSizedBindingNames is not cleaning up correctly after failure. As best I can tell, this is solved by the patches on this bug. [1] https://crash-stats.mozilla.com/signature/?product=Firefox&address=~e5e5&signature=js%3A%3Agc%3A%3AClearEdgesTracer%3A%3AclearEdge%3CT%3E&date=%3E%3D2018-10-25T19%3A45%3A16.000Z&date=%3C2018-11-01T19%3A45%3A16.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

[Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1263355 User impact if declined: GC unsafety, sec-critical. Is this code covered by automated tests?: No Has the fix been verified in Nightly?: No Needs manual test from QE?: No If yes, steps to reproduce: The test in comment 0 ought be reduced to a landable testcase, but it's difficult to do so to satisfaction to land -- and we can't land immediately anyway, so I haven't tried to run that to ground. :-\ List of other uplifts needed: None Risk to taking this patch: Medium Why is the change risky/not risky? (and alternatives if risky): See comment 17. We should land this so there's enough time to suss out issues once landed, but not too early because it reveals an exploitable issue. String changes made/needed: N/A

[ESR Uplift Approval Request] If this is not a sec:{high,crit} bug, please state case for ESR consideration: GC unsafety, sec-critical. User impact if declined: GC unsafety, sec-critical. Fix Landed on Version: Ready to land on trunk whenever safe to. Risk to taking this patch: Medium Why is the change risky/not risky? (and alternatives if risky): See comment 17, and the comment on the beta rollup as well. String or UUID changes made by this patch: N/A

[Security Approval Request] How easily could an exploit be constructed based on the patch?: See comment 12. 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?: ESR60, beta are all we care about now, correct? They're affected If not all supported branches, which bug introduced the flaw?: Bug 1263355 Do you have backports for the affected branches?: Yes If not, how different, hard to create, and risky will they be?: How likely is this patch to cause regressions; how much testing does it need?: See comment 17. We should land later in a cycle, but not *too* late. I leave the Goldilocks estimation to you.

Comment on attachment 9022811 [details] [diff] [review] Rollup patch for trunk sec-approval+ and beta approval. I'll let release management do ESR60.

Landed on trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3c1bdec432c Past history suggests there are branch checkin elves who will cover beta/esr60 when those are each ready, so I will leave them to it.

Comment on attachment 9022808 [details] [diff] [review] ESR60 rollup Approved for 60.4.0esr also.

JSBugMon: This bug has been automatically verified fixed.

JSBugMon: This bug has been automatically verified fixed on Fx64