Closed Bug 623301 Opened 11 years ago Closed 11 years ago

GC related crash in js::PropertyTable::search

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: decoder, Assigned: Waldo)

Details

(Keywords: regression, Whiteboard: [sg:critical][fixed-in-tracemonkey][hardblocker])

Attachments

(1 file, 3 obsolete files)

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
I think this is me.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
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".
Attached patch Patch (obsolete) — Splinter Review
Stupid mistake.  I really thought I'd tested this, but clearly I didn't.
Attachment #501464 - Flags: review?(jorendorff)
The previous one kind of missed the most important part!
Attachment #501464 - Attachment is obsolete: true
Attachment #501466 - Flags: review?(jorendorff)
Attachment #501464 - Flags: review?(jorendorff)
Attached patch With non-copypasta attribution (obsolete) — Splinter Review
Attachment #501466 - Attachment is obsolete: true
Attachment #501471 - Flags: review?(jorendorff)
Attachment #501466 - Flags: review?(jorendorff)
Attachment #501471 - Attachment is obsolete: true
Attachment #501472 - Flags: review?(jorendorff)
Attachment #501471 - Flags: review?(jorendorff)
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
Attachment #501472 - Flags: review?(jorendorff) → review+
WFM on old branches
(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.
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla2.0b9
http://hg.mozilla.org/tracemonkey/rev/2025b4eeca14
Whiteboard: fixed-in-tracemonkey
blocking2.0: --- → betaN+
blocking2.0: betaN+ → beta9+
I backed this out because of tinderbox failures.
Whiteboard: fixed-in-tracemonkey
Strangely, the tinderbox failures actually are somewhat due to this patch.  Not sure what's up with that, needs some thought clearly.
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?
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.
--> betaN+

We missed the beta9 boat, so it'll have to make the next one.
blocking2.0: beta9+ → betaN+
(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?
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.
Whiteboard: [sg:critical]
(this is sg critical, to confirm comment 16)
What's up with the tinderbox failures? Comment 12 left us hanging.

/be
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.
Whiteboard: [sg:critical] → [sg:critical][fixed-in-tracemonkey]
Target Milestone: mozilla2.0b9 → mozilla2.0b10
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).
Whiteboard: [sg:critical][fixed-in-tracemonkey] → [sg:critical][fixed-in-tracemonkey][hardblocker]
http://hg.mozilla.org/mozilla-central/rev/b55e045d0fc3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Fixed for a long time and not affecting old branches, opening this.
Group: core-security
Old tracer bug, marking verified.
Status: RESOLVED → VERIFIED
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.