Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash [@ js::types::TypeSet::add] with valgrind errors indicating use-after-free

VERIFIED FIXED in Firefox 14

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
8 months ago

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla15
x86_64
Linux
crash, csectype-uaf, sec-critical, testcase, valgrind
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ fixed, firefox15+ fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [sg:critical][js:p1:fx16][advisory-tracking+][qa?], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
The following test crashes on mozilla-central revision c61e7c3a232a (options -m -n -a):


gczeal(2);
function testCallProtoMethod() {
    function X() { 
        this.valueOf = new testCallProtoMethod( "return this.value" );
    } 
    X.prototype.getName = function () { return "X"; }
    var a = [new X, new X, new getName, new new this, new Y];
}
testCallProtoMethod();


Although this particular test crashes with a null pointer deref, it seems to be rather a memory corruption (original test crashed with non-null pointer). Also Valgrind shows the following errors:

==5904== Invalid read of size 8
==5904==    at 0x77B4B3: SetPropCompiler::updateMonitoredTypes() (PolyIC.cpp:480)
==5904==    by 0x77BB7E: SetPropCompiler::update() (PolyIC.cpp:613)
==5904==    by 0x7819FE: js::mjit::ic::SetProp(js::VMFrame&, js::mjit::ic::PICInfo*) (PolyIC.cpp:1911)
==5904==    by 0x6D1361: ??? (in /srv/repos/mozilla-central/js/src/debug64/shell/js)
==5904==    by 0x6D28C6: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1052)
[...]
==5904==  Address 0x720c1a8 is 1,704 bytes inside a block of size 1,736 free'd
==5904==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==5904==    by 0x4039DE: js_free (Utility.h:201)
[...]
==5904==    by 0x6D3565: js::mjit::JITScript::destroyChunk(js::FreeOp*, unsigned int, bool) (MethodJIT.cpp:1332)
==5904==    by 0x6D34A0: js::mjit::JITScript::destroy(js::FreeOp*) (MethodJIT.cpp:1319)
==5904==    by 0x6D3A72: JSScript::ReleaseCode(js::FreeOp*, JSScript::JITScriptHandle*) (MethodJIT.cpp:1456)
==5904==    by 0x4846B9: js::mjit::ReleaseScriptCode(js::FreeOp*, JSScript*) (MethodJIT.h:908)
==5904==    by 0x486E5C: JSCompartment::discardJitCode(js::FreeOp*) (jscompartment.cpp:486)
==5904==    by 0x487156: JSCompartment::sweep(js::FreeOp*, bool) (jscompartment.cpp:530)
==5904==
==5904== Invalid read of size 8
==5904==    at 0x4E40E7: js::types::TypeSet::add(JSContext*, js::types::TypeConstraint*, bool) (jsinfer.cpp:396)
==5904==    by 0x4E471A: js::types::TypeSet::addSubset(JSContext*, js::types::TypeSet*) (jsinfer.cpp:534)
==5904==    by 0x77B4C5: SetPropCompiler::updateMonitoredTypes() (PolyIC.cpp:480)
==5904==    by 0x77BB7E: SetPropCompiler::update() (PolyIC.cpp:613)
==5904==    by 0x7819FE: js::mjit::ic::SetProp(js::VMFrame&, js::mjit::ic::PICInfo*) (PolyIC.cpp:1911)
[...]
==5904==    by 0x6D2B56: CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) (MethodJIT.cpp:1111)
==5904==  Address 0x71d0568 is 29,000 bytes inside a block of size 131,072 free'd
==5904==    at 0x4C282ED: free (vg_replace_malloc.c:366)
[...]
==5904==    by 0x4C0E31: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:3929)


Marking s-s as this looks like a GC use-after-free.
Whiteboard: js-triage-needed → [sg:critical] js-triage-needed
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox14: --- → +
tracking-firefox15: --- → +
Keywords: sec-critical
Does this affect the ESR-10 branch, or is it a more recent regression?
Assignee: general → dmandelin
Christian, what build config are you using? I tried this on OSX 10.6 with -m -n -a on a debug build and got no crash on rev c61e7c3a232a and tip.
(Reporter)

Comment 3

5 years ago
(In reply to David Mandelin from comment #2)
> Christian, what build config are you using? I tried this on OSX 10.6 with -m
> -n -a on a debug build and got no crash on rev c61e7c3a232a and tip.

Should be --enable-debug --disable-optimize --enable-valgrind.

Have you tried Valgrind on it? It might not crash on every machine but the Valgrind errors might still be visible.
I reproduced this with m-c changeset 64187d60fae7:

valgrind --smc-check=all-non-file --dsymutil=yes ./js -m -n -a testcase.js

on Mac OS X 10.7 32-bit debug js shell.
status-firefox13: --- → wontfix
(Assignee)

Comment 5

5 years ago
Can you attach a full stack for the valgrind logs?
Whiteboard: [sg:critical] js-triage-needed → [sg:critical][js:p1:fx16]
Created attachment 628878 [details]
Valgrind stack

Brian, hope this Valgrind stack helps.
(Assignee)

Comment 7

5 years ago
Hi, I'm trying to see what triggers the GC, which requires a deeper stack.  Can you increase the stack depth that valgrind reports, with --num-callers ?
Created attachment 629092 [details]
Valgrind stack with --num-callers=50
Looks like all the stacks end in generated code

==60901==    by 0x2F64FDF: ???

which presumably doesn't help Brian much.  One thing you could try,
assuming you're on Linux, is to run with valgrind's built in GDB server
enabled.  Then, when you get to the error in question, you can ask GDB
"where", and maybe it can produce a better stack.  GDB has better last-ditch
unwinding heuristics than V does.

To enable the gdb server, give --vgdb-error=0 to V and follow the directions it
prints.  You need to be on Linux and using a recent V (3.7.0, or better the trunk)
(Assignee)

Comment 10

5 years ago
Created attachment 629161 [details] [diff] [review]
patch

No, this is enough stack information (thanks).  The problem is that getType() can trigger a GC and jitcode / analysis discarding when constructing the lazy type object for singletons.
Assignee: dmandelin → bhackett1024
Attachment #629161 - Flags: review?(dvander)
Attachment #629161 - Flags: review?(dvander) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/907a48923ab4
(Assignee)

Comment 12

5 years ago
Comment on attachment 629161 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): TI
User impact if declined: Potential extremely difficult to trigger security issue.
Risk to taking this patch (and alternatives if risky): None.
Attachment #629161 - Flags: approval-mozilla-beta?
Attachment #629161 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/907a48923ab4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Reporter)

Comment 14

5 years ago
JSBugMon: This bug has been automatically verified fixed.
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
Because this is TI, assuming ESR is affected.
status-firefox-esr10: --- → affected
Comment on attachment 629161 [details] [diff] [review]
patch

[Triage Comment]
Approved for Beta 14 (post-merge) since this is a low risk sg:crit fix.
Attachment #629161 - Flags: approval-mozilla-beta?
Attachment #629161 - Flags: approval-mozilla-beta+
Attachment #629161 - Flags: approval-mozilla-aurora?
status-firefox-esr10: affected → ---
status-firefox15: affected → fixed
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/dd930d15a218
status-firefox14: affected → fixed

Updated

5 years ago
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 14+
(Assignee)

Comment 18

5 years ago
Comment on attachment 629161 [details] [diff] [review]
patch

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky):  None
Attachment #629161 - Flags: approval-mozilla-esr10?
Comment on attachment 629161 [details] [diff] [review]
patch

please go ahead and land to ESR.
Attachment #629161 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 20

5 years ago
http://hg.mozilla.org/releases/mozilla-esr10/rev/f1e523032323
(Assignee)

Updated

5 years ago
status-firefox-esr10: affected → fixed
Whiteboard: [sg:critical][js:p1:fx16] → [sg:critical][js:p1:fx16][advisory-tracking+]
Christian, is there somewhere I can get builds to test this fix for Firefox 14, 15, and ESR 10.0.6? if not, how can I make some?
Whiteboard: [sg:critical][js:p1:fx16][advisory-tracking+] → [sg:critical][js:p1:fx16][advisory-tracking+][qa?]
(Reporter)

Comment 22

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #21)
> Christian, is there somewhere I can get builds to test this fix for Firefox
> 14, 15, and ESR 10.0.6? if not, how can I make some?

Right now, we cannot get beta, release or ESR builds because I cannot push these to try.

You could build them manually but then you would require the toolchain to do so on Linux/Mac (install Clang/LLVM). If that's what you want, then I can provide you with a manual.
What about Aurora? I'm not sure I can get the builds built and verified in time, given the current time window, whether I had a manual or not. That said, it would be helpful to have a manual so I can set up an environment on my own and make verification of these bugs simpler in the future.
(Reporter)

Comment 24

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #23)
> What about Aurora?

You're lucky :D, I already have nightly aurora builds for ASan at http://people.mozilla.org/~choller/firefox/asan/ :)

> it would be helpful to have a manual so I can set up an environment on
> my own and make verification of these bugs simpler in the future.

Okay. There's a wikipage but it's outdated right now. I have the update ready already but I won't be able to put it there until tomorrow. I'll ping you on IRC tomorrow with the link. If I forget it, please bug me on IRC^^
(In reply to Christian Holler (:decoder) from comment #24)
> (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #23)
> > What about Aurora?
> 
> You're lucky :D, I already have nightly aurora builds for ASan at
> http://people.mozilla.org/~choller/firefox/asan/ :)

Yeah, I knew about those builds, and I've been using them for some of these ASan builds; thanks for that. I think the problem from where I stand is getting an ASan build which reproduces the crash so that I can trust my verification.
Group: core-security
(Reporter)

Comment 26

4 years ago
Not adding the test since it's too unreliable.
Flags: in-testsuite-
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.