Last Comment Bug 746103 - Crash [@ js::types::TypeSet::add] with valgrind errors indicating use-after-free
: Crash [@ js::types::TypeSet::add] with valgrind errors indicating use-after-free
Status: VERIFIED FIXED
[sg:critical][js:p1:fx16][advisory-tr...
: crash, csectype-uaf, sec-critical, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla15
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-04-17 04:38 PDT by Christian Holler (:decoder)
Modified: 2016-12-01 13:31 PST (History)
11 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
14+
fixed


Attachments
Valgrind stack (6.96 KB, text/plain)
2012-05-31 13:22 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Valgrind stack with --num-callers=50 (11.02 KB, text/plain)
2012-06-01 00:24 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
patch (991 bytes, patch)
2012-06-01 06:11 PDT, Brian Hackett (:bhackett)
dvander: review+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-04-17 04:38:14 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2012-05-10 13:41:51 PDT
Does this affect the ESR-10 branch, or is it a more recent regression?
Comment 2 David Mandelin [:dmandelin] 2012-05-10 18:20:44 PDT
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.
Comment 3 Christian Holler (:decoder) 2012-05-10 18:22:15 PDT
(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.
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2012-05-24 12:58:53 PDT
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.
Comment 5 Brian Hackett (:bhackett) 2012-05-31 06:33:49 PDT
Can you attach a full stack for the valgrind logs?
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2012-05-31 13:22:01 PDT
Created attachment 628878 [details]
Valgrind stack

Brian, hope this Valgrind stack helps.
Comment 7 Brian Hackett (:bhackett) 2012-05-31 20:16:36 PDT
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 ?
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2012-06-01 00:24:28 PDT
Created attachment 629092 [details]
Valgrind stack with --num-callers=50
Comment 9 Julian Seward [:jseward] 2012-06-01 00:33:54 PDT
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)
Comment 10 Brian Hackett (:bhackett) 2012-06-01 06:11:41 PDT
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.
Comment 11 Brian Hackett (:bhackett) 2012-06-02 20:25:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/907a48923ab4
Comment 12 Brian Hackett (:bhackett) 2012-06-02 20:26:51 PDT
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.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-03 12:45:12 PDT
https://hg.mozilla.org/mozilla-central/rev/907a48923ab4
Comment 14 Christian Holler (:decoder) 2012-06-03 13:34:13 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 15 Alex Keybl [:akeybl] 2012-06-04 15:41:10 PDT
Because this is TI, assuming ESR is affected.
Comment 16 Alex Keybl [:akeybl] 2012-06-04 15:41:56 PDT
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.
Comment 17 Brian Hackett (:bhackett) 2012-06-07 14:33:41 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/dd930d15a218
Comment 18 Brian Hackett (:bhackett) 2012-06-22 15:08:52 PDT
Comment on attachment 629161 [details] [diff] [review]
patch

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky):  None
Comment 19 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-28 16:13:18 PDT
Comment on attachment 629161 [details] [diff] [review]
patch

please go ahead and land to ESR.
Comment 20 Brian Hackett (:bhackett) 2012-07-06 06:09:11 PDT
http://hg.mozilla.org/releases/mozilla-esr10/rev/f1e523032323
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 12:37:49 PDT
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?
Comment 22 Christian Holler (:decoder) 2012-07-11 13:48:38 PDT
(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.
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 14:05:46 PDT
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.
Comment 24 Christian Holler (:decoder) 2012-07-11 14:08:06 PDT
(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^^
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 14:26:38 PDT
(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.
Comment 26 Christian Holler (:decoder) 2013-03-11 07:25:42 PDT
Not adding the test since it's too unreliable.

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