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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Does this affect the ESR-10 branch, or is it a more recent regression?

Comment 2

[David Mandelin [: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.

Comment 3

[Christian Holler (:decoder)]

(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] (NOT official MoCo now)]

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 [Laid off!]]

Can you attach a full stack for the valgrind logs?

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Valgrind stack

Brian, hope this Valgrind stack helps.

Comment 7

[Brian Hackett [Laid off!]]

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] (NOT official MoCo now)]

Attachment: Valgrind stack with --num-callers=50

Comment 9

[Julian Seward [:jseward]]

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 [Laid off!]]

Attachment: 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 [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/907a48923ab4

Comment 12

[Brian Hackett [Laid off!]]

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: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/907a48923ab4

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Alex Keybl [:akeybl]]

Because this is TI, assuming ESR is affected.

Comment 16

[Alex Keybl [:akeybl]]

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 [Laid off!]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd930d15a218

Comment 18

[Brian Hackett [Laid off!]]

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]

Comment on attachment 629161 [details] [diff] [review]
patch

please go ahead and land to ESR.

Comment 20

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/f1e523032323

Comment 21

[u279076]

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

(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

[u279076]

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

(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

[u279076]

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

Not adding the test since it's too unreliable.