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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

Looking through the list of commits, bug 1141865 looks most related. Eric, can you please take a look?

Comment 3

[Eric Faust [:efaust]]

Attachment: Fix

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.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision b7ee8e13145a).

Comment 5

[Jan de Mooij [:jandem]]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

This probably shouldn't have landed without security approval and with a test included.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Also, backed out for suspicious-looking JS crashes/failures since it landed.
https://treeherder.mozilla.org/logviewer.html#?job_id=11030147&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11032170&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=11031692&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3fddb7a3eb

Comment 7

[Al Billings [:abillings - ex-MoCo]]

(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?

Comment 8

[Eric Faust [:efaust]]

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.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Cool. Thanks! No sec-approval needed then, as you noted.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 11

[Eric Faust [:efaust]]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Let's get an aurora nomination as well.

Comment 13

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 14

[Eric Faust [:efaust]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please get this landed on inbound ASAP. It can't be uplifted to Aurora until it's fixed on trunk.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6a517d18f12

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f72ae71dda1a

Comment 20

[Christian Holler (:decoder)]

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 :)

Comment 21

[Christian Holler (:decoder)]

Fwiw, reported as a second issue in bug 1180067.