623301 - GC related crash in js::PropertyTable::search

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code will crash on TM tip:

options('tracejit');
gczeal(2);

function crashMe(n) {
  nasty = [];
  while (n--)
    nasty.push("a"+n);
  fn = Function.apply(null, nasty);
}
crashMe(0x8001);


Flagged security because apparently some object is being used although it was freed before:

Program received signal SIGSEGV, Segmentation fault.
0x000000000055c0f6 in js::PropertyTable::search (this=0xdadadadadadadada, id=..., adding=false) at jsscope.cpp:303
303         JS_ASSERT(entries);
(gdb) bt
#0  0x000000000055c0f6 in js::PropertyTable::search (this=0xdadadadadadadada, id=..., adding=false) at jsscope.cpp:303
#1  0x0000000000434e10 in js::Shape::search (rt=0x7ffff7fc3010, startp=0x7fffffffba50, id=..., adding=false) at jsscope.h:859
#2  0x0000000000561e84 in js::Bindings::lookup (this=0x7fffffffba50, cx=0xaa1d70, name=0x7ffff6232040, indexp=0x0) at jsscript.cpp:84
#3  0x00000000004904d5 in js::Bindings::hasBinding (this=0x7fffffffba50, cx=0xaa1d70, name=0x7ffff6232040) at jsscript.h:265
#4  0x00000000004a1458 in Function (cx=0xaa1d70, argc=32769, vp=0x7ffff6abf138) at jsfun.cpp:2499
#5  0x00000000004c92c6 in js::CallJSNative (cx=0xaa1d70, native=0x4a0b68 <Function>, argc=32769, vp=0x7ffff6abf138) at jscntxtinlines.h:685
#6  0x00000000004c5712 in js::Invoke (cx=0xaa1d70, argsRef=..., flags=0) at jsinterp.cpp:700
#7  0x00000000004a0620 in js_fun_apply (cx=0xaa1d70, argc=2, vp=0x7ffff6abf118) at jsfun.cpp:2143
#8  0x00000000004c92c6 in js::CallJSNative (cx=0xaa1d70, native=0x4a02ea <js_fun_apply(JSContext*, unsigned int, js::Value*)>, argc=2, vp=0x7ffff6abf118) at jscntxtinlines.h:685

Comment 1

[Jeff Walden [:Waldo]]

I think this is me.

Comment 2

[Christian Holler (:decoder)]

The following test case seems related:

gczeal(2);
var foo = "asdf";
new Function(foo, foo, foo);


It crashes slightly differently:

0x000000000055ff8b in js::PropertyTable::capacity (this=0xdadadadadadadada) at jsscope.h:249

but both are in PropertyTable and the test cases both refer to "Function".

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

Stupid mistake.  I really thought I'd tested this, but clearly I didn't.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Firefox should warn me when I attach an un-qref'd patch to a bug

The previous one kind of missed the most important part!

Comment 5

[Jeff Walden [:Waldo]]

Attachment: With non-copypasta attribution

Comment 6

[Jeff Walden [:Waldo]]

Attachment: See patch #2's description

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 501472 [details] [diff] [review]
See patch #2's description

Drive-by, I need jorendorff on bug 592202 and I've read this code too.

/be

Comment 8

[Daniel Veditz [:dveditz]]

WFM on old branches

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #8)
> WFM on old branches

Yes, this is the result of a very recent reorganization (last two weeks, really) -- before then these properties were kept alive by the rooted (probably via vp[0] or something) function to be eventually returned.

Comment 10

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/2025b4eeca14

Comment 11

[Robert Sayre]

I backed this out because of tinderbox failures.

Comment 12

[Jeff Walden [:Waldo]]

Strangely, the tinderbox failures actually are somewhat due to this patch.  Not sure what's up with that, needs some thought clearly.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

This was set to blocking beta9, which makes me think that it's required to compartment-GC, but didn't get checked into the tree in time for builds.

What's up?

Comment 14

[Andreas Gal :gal]

This is unrelated to compartments. Its very low risk row. We should be able to take it for b9, if we want to. We don't _have_ to though.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

--> betaN+

We missed the beta9 boat, so it'll have to make the next one.

Comment 16

[Christian Holler (:decoder)]

(In reply to comment #14)

> Its very low risk row.

I thought this bug was a use after free. I don't really see how a use after free is low risk in any case or am I missing something?

Comment 17

[Andreas Gal :gal]

Sorry, that sentence got scrambled in the middle. Its low risk to TAKE. Not to exploit. I am confident this will not break anything if we add it to the b9 mix.

Comment 18

[Andreas Gal :gal]

(this is sg critical, to confirm comment 16)

Comment 19

[Brendan Eich [:brendan]]

What's up with the tinderbox failures? Comment 12 left us hanging.

/be

Comment 20

[Jeff Walden [:Waldo]]

If bug 624640 had been fixed, this would have landed cleanly.  I only just got to figuring that out yesterday, too late to push, but now it's in:

http://hg.mozilla.org/tracemonkey/rev/b55e045d0fc3

I added the extra gczeal call to the test before bug 624640 got reviewed, and when that got r+ I didn't bother with a change (belt-and-suspenders, at this point), so this has an extra gczeal call that's not truly necessary any more but won't hurt.

Comment 21

[Jeff Walden [:Waldo]]

Oh, this had nothing to do with compartments -- just a simple GC hazard with a simple fix (and a slightly-harder-to-write test, but the changes themselves are simple).

Comment 22

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/b55e045d0fc3

Comment 24

[Christian Holler (:decoder)]

Fixed for a long time and not affecting old branches, opening this.

Comment 25

[Christian Holler (:decoder)]

Old tracer bug, marking verified.