Closed Bug 737384 Opened 8 years ago Closed 8 years ago

Assertion failure: thing, at js/src/jsgcmark.cpp:7 or Crash [@ js::gc::MarkInternal]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox12 + fixed
firefox13 + fixed
firefox14 + fixed
firefox-esr10 12+ verified

People

(Reporter: decoder, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical][qa-] js-triage-done)

Crash Data

Attachments

(1 file)

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.
Attached patch fixSplinter Review
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?
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #607768 - Flags: review?(luke)
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 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.
Attachment #607768 - Flags: review?(luke) → review+
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.
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.
(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.
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
Whiteboard: [sg:critical] js-triage-needed → [sg:critical] js-triage-done
Target Milestone: --- → mozilla14
> 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?
https://hg.mozilla.org/mozilla-central/rev/4f5d0ac5f3a6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
[Triage Comment]
Please nominate the fix for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
A fixed address that an attacker could realistically target is pretty bad.
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
Attachment #607768 - Flags: approval-mozilla-esr10?
Attachment #607768 - Flags: approval-mozilla-beta?
Attachment #607768 - Flags: approval-mozilla-aurora?
Comment on attachment 607768 [details] [diff] [review]
fix

[Triage Comment]
Low risk and sg:crit, approved for all branches.
Attachment #607768 - Flags: approval-mozilla-esr10?
Attachment #607768 - Flags: approval-mozilla-esr10+
Attachment #607768 - Flags: approval-mozilla-beta?
Attachment #607768 - Flags: approval-mozilla-beta+
Attachment #607768 - Flags: approval-mozilla-aurora?
Attachment #607768 - Flags: approval-mozilla-aurora+
Whiteboard: [sg:critical] js-triage-done → [sg:critical][qa+] js-triage-done
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.
(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?
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.
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?
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?
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.
Given comment 20, I don't think this is something QA can verify so marking qa-. Please correct me if I am wrong.
Whiteboard: [sg:critical][qa+] js-triage-done → [sg:critical][qa-] js-triage-done
Group: core-security
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug737384.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.