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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

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]]

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]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

(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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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]

> 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]]

https://hg.mozilla.org/mozilla-central/rev/4f5d0ac5f3a6

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

[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]]

A fixed address that an attacker could realistically target is pretty bad.

Comment 12

[Daniel Veditz [:dveditz]]

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]]

Comment on attachment 607768 [details] [diff] [review]
fix

[Triage Comment]
Low risk and sg:crit, approved for all branches.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e158b91dd28a
https://hg.mozilla.org/releases/mozilla-beta/rev/e6be81269d27
https://hg.mozilla.org/releases/mozilla-esr10/rev/07c08a7dc1ea

Comment 15

[u279076]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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

[u279076]

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]]

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

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[u279076]

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

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug737384.js.