__defineGetter__("eval", decodeURI); f = (function() { function g(i0, i1) { i = i0 | 0 i1 = i1 | 0; (3 % (1 ^ i))() g() arguments } return g })() x = [, , eval] for (var j = 0; j < 9; ++j) { for (var k = 0; k < 9; ++k) { try { f(x[j], x[k]); } catch (e) {} } } crashes js debug and opt shell on m-c changeset eabe3f50b083 with --ion-eager --ion-parallel-compile=off at js::assertSameCompartment and js::ScopedThreadSafeStringInspector::ensureChars respectively. My configure flags are: (opt) LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options> (Debug): LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options> This seems to be a null-deref, but setting s-s to be safe first.

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/2963a336e7ec user: Kannan Vijayan date: Mon Sep 30 10:24:30 2013 -0400 summary: Bug 921120 - Enable Ion-compilation of JSOP_SETARG for functions which use magic arguments. r=nbp Kannan, is bug 921120 a likely regressor?

Looked into this briefly. We're constructing a StringValue with a null payload, which triggers the assert. The bad construction is happening somewhere in Ion code, and is visible during bailout to baseline. This may be related to bug 943366. Taking.

I instrumented all storeValues and loadValues to check for null strings, and they don't trigger at all. However the bad string is getting into the snapshot, and it doesn't seem to be explicitly stored by jitcode. This may be a snapshots bug. I tried to use lldb to iterate over the snapshot code, but I can't because lldb sucks and can't read frame variables which are clearly not optimized out. I might have to break out the printf debugging on this. Sigh.

Codegen for offending function.

After some digging, I've discovered the following truths about this bug. See the codegen.png attachment above for what I'm referring to. We're bailing to a resume point somewhere between instruction 14 (storeslott) and instruction 15 (valuetoint32). I'm not sure if it's happening before or after the movegroup between them. The bailout is occurring at instruction 25 (unbox - fallible unbox-to-object to be specific). When resuming, the snapshot being used indicates that the value of argument 1 (which is the "bad string" that gets created on bailout) is located at [tag=edi, payload=stack(-32)]. The tag in $edi is still a string, but the payload being loaded from the stack location is a 0. Jan hypothesized that the stack slot being used is actually the frame arguments slot (would make sense, given the negative offset). If that's the case, then the issue is that the argument frame slot is getting overwritten with an int32-value. However, it's restoring the type tag from the edi register, instead of also getting the type tag from the stack slot.

Tracks whether the current graph modifies frame arguments, and uses that to skip spill-at-definition for argument values in LSRA. Fixes bug. Not including test case in this patch. This seems like a more serious security bug as it might allow attackers to construct arbitrary pointers to the heap (in this case it was string with a null payload, but the same vector could be used to put a controlled non-zero value there).

(In reply to Kannan Vijayan [:djvj] from comment #5) > We're bailing to a resume point somewhere between instruction 14 > (storeslott) and instruction 15 (valuetoint32). I'm not sure if it's > happening before or after the movegroup between them. I made a patch a while ago to display the resume points. Try to update iongraph. Otherwise this should be easy to add. This should display a gray line in between instructions with the ordered list of captured operands, and the link to the parent block in case of inlined blocks. The resume point used is supposed to be the last one before the bailing instruction. (In reply to Kannan Vijayan [:djvj] from comment #5) > When resuming, the snapshot being used indicates that the value of argument > 1 (which is the "bad string" that gets created on bailout) is located at > [tag=edi, payload=stack(-32)]. The tag in $edi is still a string, but the > payload being loaded from the stack location is a 0. We should add assertions in SnapshotIterator::slotValue to ensure that we are not producing bad values. (In reply to Kannan Vijayan [:djvj] from comment #5) > Jan hypothesized that the stack slot being used is actually the frame > arguments slot (would make sense, given the negative offset). I agree with Jan, I can also add that this is the offset to the first argument, as a JS frame contains 4 fields of 8 bytes. (In reply to Kannan Vijayan [:djvj] from comment #5) > However, it's restoring the type > tag from the edi register, instead of also getting the type tag from the > stack slot. This is the expected behavior. We are not always spilling complete things on the stack, otherwise we would have the same problem as baseline, with some stack explosion and some extra spilling. Still arguments should remain as valid Values, so only in the case of arguments, we could assert that the type-tag which is on the stack is consistent with the the type-tag extracted from the Snapshot.

Comment on attachment 8350328 [details] [diff] [review] fix-bug-951528.patch Review of attachment 8350328 [details] [diff] [review]: ----------------------------------------------------------------- Good catch. Please make sure it also fixes bug 951970 (may require an opt build, can also just download m-i builds from tbpl after landing). ::: js/src/jit/LinearScan.cpp @@ +766,5 @@ > return false; > } > } > > + bool trySpillAtDefinition = allocation.isMemory(); Nit: it's not necessarily the definition, maybe bool useAsCanonicalSpillSlot?

(In reply to Nicolas B. Pierron [:nbp] from comment #7) > We should add assertions in SnapshotIterator::slotValue to ensure that we > are not producing bad values. Thanks for the input nicolas. I'll add this check. Another set of asserts I added to help track this down was in |MacroAssembler::storeValue|, |MacroAssembler::loadValue|, and |MacroAssembler::pushValue|, testing the stored and loaded values to ensure that they were valid pointers. We can check these to ensure that we're not producing a null-pointer-to-heap, but checking "full validity" will be extremely heavyweight. Even modifying the masm output for every storeValue, loadValue and pushValue is a bit intrusive in terms of overhead, even on debug builds. It'd be nice if there was a DEBUG_HEAVY build option where we could add more heavyweight checking code.

JSBugMon: This bug has been automatically verified fixed.

Setting needinfo? so that we don't forget to backport this to aurora/beta. (Next time request sec-approval before landing.)

Posting rebased patch for aurora uplift.

Comment on attachment 8354868 [details] [diff] [review] uplift-bug951528.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 921120 User impact if declined: Exploitable issue where attacker can construct JSObject or JSString pointers with arbitrary addresses as targets. Testing completed (on m-c, etc.): Green on m-c for a week. Risk to taking this patch (and alternatives if risky): Low. String or IDL/UUID changes made by this patch: N/A

Added rebased uplift patch for beta.

Comment on attachment 8354912 [details] [diff] [review] uplift-beta-951528.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 921120 User impact if declined: Exploitable attack against JS engine where attacker can construct JSObject or JSString values with controlled addresses. Testing completed (on m-c, etc.): Green on m-c for a week. Risk to taking this patch (and alternatives if risky): Low. String or IDL/UUID changes made by this patch: N/A

https://hg.mozilla.org/releases/mozilla-aurora/rev/45a275782d8a https://hg.mozilla.org/releases/mozilla-beta/rev/7630e7db3c8d

No longer seeing this bugs summary crash signatures. However, the signature from duped bug 943366, [@ js::ExclusiveContext::getNewType(js::Class const*, js::TaggedProto, JSFunction*)], is still happening on 27b4, though in greatly reduced volume from what it was on 27beta2. I'll reopen the duped bug, if that signature doesn't drop out of topcrash status by the time we go to beta7 (next Fri).