Last Comment Bug 737384 - Assertion failure: thing, at js/src/jsgcmark.cpp:7 or Crash [@ js::gc::MarkInternal]
: Assertion failure: thing, at js/src/jsgcmark.cpp:7 or Crash [@ js::gc::MarkIn...
Status: RESOLVED FIXED
[sg:critical][qa-] js-triage-done
: assertion, crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla14
Assigned To: [PTO to Dec5] Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-03-20 05:28 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 08:25 PST (History)
10 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
12+
verified


Attachments
fix (1.53 KB, patch)
2012-03-20 16:27 PDT, [PTO to Dec5] Bill McCloskey (:billm)
luke: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-03-20 05:28:56 PDT
The following test asserts/crashes on mozilla-central revision 58a2cd0203ee (options -m -n):


a = evalcx('');
for (var i = 0; i < 1000; i++) {
  a[i] = i;
}
function foo(x) {
  for (var i in x) 
        var summary = "Basic support for iterable objects and for-in";
}
schedulegc(1234);
foo(a);


Crash trace:

==8882== Invalid read of size 8
==8882==    at 0x45B745: void js::gc::MarkInternal<JSString>(JSTracer*, JSString**) (jsgc.h:674)
==8882==    by 0x45C5FE: js::gc::MarkIdInternal(JSTracer*, long*) (jsgcmark.cpp:275)
==8882==    by 0x45C65E: js::gc::MarkIdRootRange(JSTracer*, unsigned long, long*, char const*) (jsgcmark.cpp:314)
==8882==    by 0x452E3E: JS::AutoGCRooter::trace(JSTracer*) (jsgc.cpp:2280)
==8882==    by 0x4530DA: JS::AutoGCRooter::traceAll(JSTracer*) (jsgc.cpp:2320)
==8882==    by 0x455623: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.239 (jsgc.cpp:2336)
==8882==    by 0x455F3B: BeginMarkPhase(JSRuntime*) (jsgc.cpp:2989)
==8882==    by 0x4594D5: GCCycle(JSContext*, JSCompartment*, long, js::JSGCInvocationKind) (jsgc.cpp:3295)
==8882==    by 0x45A51E: Collect(JSContext*, JSCompartment*, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3739)
==8882==    by 0x45A835: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:3760)
==8882==    by 0x4D1FE4: _ZN2js2gc10NewGCThingI8JSStringEEPT_P9JSContextNS0_9AllocKindEm.clone.228 (jsgcinlines.h:415)
==8882==    by 0x4D462C: js_NewStringCopyN(JSContext*, unsigned short const*, unsigned long) (jsgcinlines.h:474)
==8882==  Address 0xfc0b8 is not stack'd, malloc'd or (recently) free'd



I confirmed this is not bug 733979, or at least this issue is not fixed by the patch in that bug :) Assuming s-s and sg:critical due to GC-related crash at non-zero address.
Comment 1 [PTO to Dec5] Bill McCloskey (:billm) 2012-03-20 16:27:43 PDT
Created attachment 607768 [details] [diff] [review]
fix

Hmm, a bug from long ago. After the resize op, the vector contained uninitialized data that was scanned by the GC.

I wonder if schedulegc is what exposed this to the fuzzers? Christian, do you remember when you started fuzzing with that one?
Comment 2 Christian Holler (:decoder) 2012-03-20 16:33:45 PDT
I added schedulegc to the list of builtin functions as soon as you added it. But maybe this test is derived from a recently added test, or just very hard to find :)
Comment 3 Luke Wagner [:luke] 2012-03-20 17:18:31 PDT
Comment on attachment 607768 [details] [diff] [review]
fix

The fix is good, but I'm pretty sure this isn't a crash because of unitialized memory: resize() will initialize to 0, which will be interpreted as a string jsid.  Did you see a segfault at 0 bill?  Assuming I'm right, I think this would not be s-s as it is a safe crash at a low address.

On a side-note, if "int" had the low bit unset, then 0 would be interpreted as the int 0, which would be safe for GC.
Comment 4 Daniel Veditz [:dveditz] 2012-03-22 13:28:47 PDT
On the assumption that it /is/ exploitable we should land this in Fx12 (current beta) because landing a patch switching to .infallibleAppend() might attract attention.
Comment 5 [PTO to Dec5] Bill McCloskey (:billm) 2012-03-23 17:44:46 PDT
After looking at the code for Vector, I agree with Luke that the memory should be zero-initialized. What's weird is that the run in comment 0 seems to crash at a non-zero address. (When I ran it, I asserted with a zero address.) How did you run that, Christian? If I run in Valgrind, it just prints the assert and exits. If I use an opt shell, it says that schedulegc isn't defined.
Comment 6 Christian Holler (:decoder) 2012-03-23 17:48:13 PDT
(In reply to Bill McCloskey (:billm) from comment #5)
> How did you run that, Christian? If I run in Valgrind, it just
> prints the assert and exits. If I use an opt shell, it says that schedulegc
> isn't defined.

This is a valgrind run with an opt shell, compiled with these options:

--disable-debug --enable-optimize --enable-valgrind --enable-gczeal

I just verified that this still crashes at non-zero address in opt build.
Comment 7 [PTO to Dec5] Bill McCloskey (:billm) 2012-03-26 11:11:56 PDT
Thanks, Christian. I looked at this a little more, and it turns out that the address it's crashing on is the address of the mark bit for NULL. So it will always be the same low address (0xfc0b8, just below 1MB), which I think is pretty safe. Consequently, I think this should be sg:dos. Is that correct?

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5d0ac5f3a6
Comment 8 Jesse Ruderman 2012-03-26 12:29:14 PDT
> So it will always be the same low address (0xfc0b8, just below 1MB), which I think 
> is pretty safe.

I'm comfortable relying on the first page of memory never being mapped, but I'm not comfortable doing the same for 0xfc0b8.  Dan, do you know our tier-1 platforms behave?
Comment 9 Ed Morley [:emorley] 2012-03-27 05:39:57 PDT
https://hg.mozilla.org/mozilla-central/rev/4f5d0ac5f3a6
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-06 15:04:37 PDT
[Triage Comment]
Please nominate the fix for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Comment 11 Daniel Veditz [:dveditz] 2012-04-12 13:52:12 PDT
A fixed address that an attacker could realistically target is pretty bad.
Comment 12 Daniel Veditz [:dveditz] 2012-04-12 13:53:59 PDT
Comment on attachment 607768 [details] [diff] [review]
fix

[Approval Request Comment]
User impact if declined: possible security vulnerability
Testing completed (on m-c, etc.): two weeks of nightly testing, verification
Risk to taking this patch (and alternatives if risky):
String changes made by this patch: none
Comment 13 Alex Keybl [:akeybl] 2012-04-12 15:12:45 PDT
Comment on attachment 607768 [details] [diff] [review]
fix

[Triage Comment]
Low risk and sg:crit, approved for all branches.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-16 11:16:16 PDT
Having problems trying to reproduce this; I get "schedulegc is not defined" errors. Do I need to build a special shell? Trying to test with 2012-04-16 mozilla-esr10 js-shell.
Comment 16 [PTO to Dec5] Bill McCloskey (:billm) 2012-04-16 11:18:41 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #15)
> Having problems trying to reproduce this; I get "schedulegc is not defined"
> errors. Do I need to build a special shell? Trying to test with 2012-04-16
> mozilla-esr10 js-shell.

Are you using a debug shell?
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-16 11:30:25 PDT
gkw on IRC suggested I use the following:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr10-linux64-debug/1334598618/jsshell-linux-x86_64.zip

Using that I am able to execute the test successfully; no crashes nor assertions.
Comment 18 Mike Hommey [:glandium] 2012-04-17 11:35:14 PDT
Is it really fixed on esr ? because the changeset is empty...
https://hg.mozilla.org/releases/mozilla-esr10/rev/07c08a7dc1ea

Or was esr not affected to begin with?
Comment 19 Christian Holler (:decoder) 2012-04-17 12:28:30 PDT
I tried the test from comment 0 on esr10 and it does not assert. That either means it was never affected or the test does not reproduce the issue there in general.

Bill, can you check this?
Comment 20 [PTO to Dec5] Bill McCloskey (:billm) 2012-04-18 09:18:00 PDT
That's embarrassing. I somehow screwed up the push. Anyway, here's the right fix:

https://hg.mozilla.org/releases/mozilla-esr10/rev/665a4cfb95cf

I don't think the testcase will reproduce on branches, with or without the fix. It relied on cross-compartment exceptions to trigger the incorrect behavior. However, the code is still broken.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-18 09:47:18 PDT
Given comment 20, I don't think this is something QA can verify so marking qa-. Please correct me if I am wrong.
Comment 22 Christian Holler (:decoder) 2013-01-14 08:25:25 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug737384.js.

Note You need to log in before you can comment on or make changes to this bug.