Closed Bug 1172498 Opened 4 years ago Closed 4 years ago

Crash [@ js::jit::AssertValidObjectPtr] or Crash [@ JSObject::allocKindForTenure]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-master --- fixed

People

(Reporter: decoder, Assigned: efaust)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fuzzblocker] [jsbugmon:testComment=10,origRev=3ffdf460b44a][post-critsmash-triage])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 7d4ab4a9febd (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe):

for(var e=1.2; e<10000; e++) {
  if (new (function  (c) { eval("var y"); }))
    var y=2;
}


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::AssertValidObjectPtr (cx=0x7ffff691b4e0, obj=0x7ffff4efff60) at js/src/jsobj.h:158
#0  js::jit::AssertValidObjectPtr (cx=0x7ffff691b4e0, obj=0x7ffff4efff60) at js/src/jsobj.h:158
#1  0x00007ffff7ff7223 in ?? ()
#2  0x00007fffffffc7d8 in ?? ()
#3  0x00007fffffffc628 in ?? ()
#4  0x00007fffffffc6c8 in ?? ()
#5  0xfffc7ffff4efff60 in ?? ()
#6  0x412fffd000000000 in ?? ()
#7  0xfffc000000003636 in ?? ()
#8  0x3fe002d804440666 in ?? ()
#9  0x495845534a007325 in ?? ()
#10 0x3fe002d804440666 in ?? ()
#11 0x0000000000000000 in ?? ()
rax	0xfffc000000005656	-1125899906820522
rbx	0x7ffff4efff60	140737302757216
rcx	0xfffbffffffffffff	-1125899906842625
rdx	0x7ffff693c000	140737330266112
rsi	0x7ffff4efff60	140737302757216
rdi	0x7ffff691b4e0	140737330132192
rbp	0x7fffffffc600	140737488340480
rsp	0x7fffffffc5e0	140737488340448
r8	0x7ffff69f8708	140737331037960
r9	0x7	7
r10	0x7ffff69f8710	140737331037968
r11	0x7ffff693c168	140737330266472
r12	0x7ffff691b4e0	140737330132192
r13	0x7fffffffcba0	140737488341920
r14	0x7ffff4d5a060	140737301028960
r15	0x7fffffffc870	140737488341104
rip	0xa27b67 <js::jit::AssertValidObjectPtr(JSContext*, JSObject*)+23>
=> 0xa27b67 <js::jit::AssertValidObjectPtr(JSContext*, JSObject*)+23>:	mov    0x10(%rax),%r13
   0xa27b6b <js::jit::AssertValidObjectPtr(JSContext*, JSObject*)+27>:	mov    0x8(%rdi),%rax


Marking s-s due to possibly invalid object pointer.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150604061136" and the hash "81fe755dfd47".
The "bad" changeset has the timestamp "20150604062845" and the hash "dbc89e025b5f".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=81fe755dfd47&tochange=dbc89e025b5f
Whiteboard: [jsbugmon:update] → [jsbugmon:update]]
Looking through the list of commits, bug 1141865 looks most related. Eric, can you please take a look?
Flags: needinfo?(efaustbmo)
Whiteboard: [jsbugmon:update]] → [jsbugmon:update][fuzzblocker]
Attached patch FixSplinter Review
eval inside a function does two things: it creates enough extra callobjects and stuff to force GC over the course of many iterations, and it forces an arguments object for the enclosing function.

Since we were marking all the arguments inside the arguments object in the snapshot, we were never making the overflows in MarkThisAndArguments, and thus, never marking new.target. Since new.target is not in the snapshot, this patch marks it universally inside MarkThisAndArguments.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Flags: needinfo?(efaustbmo)
Attachment #8621923 - Flags: review?(jdemooij)
Whiteboard: [jsbugmon:update][fuzzblocker] → [fuzzblocker] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision b7ee8e13145a).
Comment on attachment 8621923 [details] [diff] [review]
Fix

Review of attachment 8621923 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, good find.

::: js/src/jit/JitFrames.cpp
@@ +1049,5 @@
>  
>      // Trace |this|.
>      TraceRoot(trc, argv, "ion-thisv");
>  
>      // Trace actual arguments and newTarget beyond the formals. Note + 1 for thisv.

Nit: can now remove the "and newTarget" part.
Attachment #8621923 - Flags: review?(jdemooij) → review+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> This probably shouldn't have landed without security approval and with a
> test included.
> https://wiki.mozilla.org/Security/Bug_Approval_Process

Actually, I think this is trunk only based on the regression range identified. Eric and Decoder can confirm that. Guys?
Flags: needinfo?(efaustbmo)
Flags: needinfo?(choller)
I looked at the range, concluded it was Trunk only (which I still believe it is, as long as I reland before uplift) and landed. I did consider sec-approval, but didn't apply for it for this reason.
Flags: needinfo?(efaustbmo)
Cool. Thanks! No sec-approval needed then, as you noted.
Flags: needinfo?(choller)
gczeal(2);
for (var x = 0; x < 99; x++) {
    (function() {
        return function() {
            new function(y) {
                return {
                    e: q => q, function() {}
                }
            }
        }
    })()()
}

Here's another testcase that crashes debug builds at js::jit::AssertValidObjectPtr with --fuzzing-safe --no-threads --ion-eager --ion-inlining=off. This seems to be fixed by the patch, however, it's causing numerous intermittent crashes too so it will be nice if this is landed soon.

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Flags: needinfo?(efaustbmo)
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update,testComment=10,origRev=3ffdf460b44a]
Comment on attachment 8621923 [details] [diff] [review]
Fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Pretty easily. The test is basically a minimal failure case.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The test should maybe not land. I can also obscure some of the comments, if necessary, but would love to keep them as is. It's a missing marking site, so any changes to the marking code are gonna make that apparent.

Which older supported branches are affected by this flaw?

Just aurora currently. I missed the uplift yesterday.

If not all supported branches, which bug introduced the flaw?

bug 1141865

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Uplift was yesterday. This code is pretty stable. The patch in bug will likely apply.

How likely is this patch to cause regressions; how much testing does it need?

It's very straightforwardly marking a value that was missed in one case. It shouldn't be a problem.
Flags: needinfo?(efaustbmo)
Attachment #8621923 - Flags: sec-approval?
sec-approval+ for trunk. Let's get an aurora nomination as well.
Attachment #8621923 - Flags: sec-approval? → sec-approval+
Whiteboard: [fuzzblocker] [jsbugmon:update,testComment=10,origRev=3ffdf460b44a] → [fuzzblocker] [jsbugmon:testComment=10,origRev=3ffdf460b44a]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Comment on attachment 8621923 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]:
bug 1141865
[User impact if declined]:
GC related crashes
[Describe test coverage new/current, TreeHerder]:
Two tests landed on inbound with patch.
[Risks and why]: 
Just removes attack surface and improves GC correctness.
[String/UUID change made/needed]:
None.
Attachment #8621923 - Flags: approval-mozilla-aurora?
Attachment #8621923 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please get this landed on inbound ASAP. It can't be uplifted to Aurora until it's fixed on trunk.
Flags: needinfo?(efaustbmo)
Oh, you already did. In that case, please don't forget to mark the bug in the future when you do. Note that pulsebot can't comment in s-s bugs.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a517d18f12
Flags: needinfo?(efaustbmo) → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c6a517d18f12
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Actually the last comment was supposed to go to bug 1172478 which has almost the same signature as this one. Eric, can you please check if these are the same bug and why the test I just posted still reproduces? If this is another bug independent from this one, let me know and I can repost it as a new bug, or feel free to do this yourself :)
Flags: needinfo?(efaustbmo)
Fwiw, reported as a second issue in bug 1180067.
Flags: needinfo?(efaustbmo)
Duplicate of this bug: 1172478
Duplicate of this bug: 1172482
Whiteboard: [fuzzblocker] [jsbugmon:testComment=10,origRev=3ffdf460b44a] → [fuzzblocker] [jsbugmon:testComment=10,origRev=3ffdf460b44a][post-critsmash-triage]
Group: core-security → core-security-release
Group: core-security-release
Duplicate of this bug: 1180067
You need to log in before you can comment on or make changes to this bug.