The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 12

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: billm)

Tracking

(Blocks: 1 bug, {assertion, crash, testcase})

Trunk
mozilla14
x86_64
Linux
assertion, crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12+ fixed, firefox13+ fixed, firefox14+ fixed, firefox-esr1012+ verified)

Details

(Whiteboard: [sg:critical][qa-] js-triage-done, crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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?
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #607768 - Flags: review?(luke)
(Reporter)

Comment 2

5 years ago
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

5 years ago
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.
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox-esr10: --- → 12+
tracking-firefox12: --- → +
tracking-firefox13: --- → +
tracking-firefox14: --- → +
(Assignee)

Comment 5

5 years ago
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.
(Reporter)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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

Comment 8

5 years ago
> 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
Last Resolved: 5 years ago
status-firefox14: affected → fixed
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+
(Assignee)

Comment 14

5 years ago
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
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
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.
(Assignee)

Comment 16

5 years ago
(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.
status-firefox-esr10: fixed → verified
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?
(Reporter)

Comment 19

5 years ago
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?
(Assignee)

Comment 20

5 years ago
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
(Reporter)

Comment 22

4 years ago
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.