Last Comment Bug 766430 - Uninitialized local variable with childNodes, lookupGetter
: Uninitialized local variable with childNodes, lookupGetter
Status: RESOLVED DUPLICATE of bug 767273
[advisory-tracking-][sg:dupe 767273]
: assertion, regression, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: ---
Assigned To: Andrew McCreight [:mccr8]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 326633 762432
  Show dependency treegraph
Reported: 2012-06-19 19:16 PDT by Jesse Ruderman
Modified: 2013-01-04 18:42 PST (History)
6 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (asserts fatally when loaded) (96 bytes, text/html)
2012-06-19 19:16 PDT, Jesse Ruderman
no flags Details
updated testcase (141 bytes, text/html)
2012-06-21 03:09 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
testcase with setTimeout (178 bytes, text/html)
2012-06-25 13:07 PDT, Jesse Ruderman
no flags Details
testcase that tries harder (227 bytes, text/html)
2012-06-25 13:14 PDT, Jesse Ruderman
no flags Details
stack trace 1 (5.74 KB, text/plain)
2012-07-09 15:30 PDT, Jesse Ruderman
no flags Details
root moar, still needs a test (1.07 KB, patch)
2012-07-13 14:09 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
this got a little out of hand, WIP (14.10 KB, patch)
2012-07-15 14:21 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-19 19:16:07 PDT
Created attachment 634712 [details]
testcase (asserts fatally when loaded)
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-06-20 05:42:32 PDT
I can't reproduce this on my trunk build (debug, macosx64). Jesse, any chance this was fixed by bug 764289?
Comment 2 Jesse Ruderman 2012-06-20 11:13:57 PDT
Hrm, it still asserts/crashes for me, with a blank profile, both with and with a local build (mozilla-central-97119-e240d6e43c9a-debug).

Do you want to debug on my computer?
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-21 03:09:08 PDT
Created attachment 635232 [details]
updated testcase

I had to flesh the testcase out a little bit to get it to crash. Updating.
Comment 4 Jesse Ruderman 2012-06-22 12:44:34 PDT
And now I need a setTimeout to reproduce :/
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-06-25 03:47:56 PDT
(In reply to Jesse Ruderman from comment #4)
> And now I need a setTimeout to reproduce :/

I can't reproduce with my old testcase. Can you still reproduce with your setTimeout version? If so, can you upload it? If not, I can rewind by repo and debug there.
Comment 6 Jesse Ruderman 2012-06-25 13:07:36 PDT
Created attachment 636461 [details]
testcase with setTimeout
Comment 7 Jesse Ruderman 2012-06-25 13:14:41 PDT
Created attachment 636466 [details]
testcase that tries harder

This one requires
Comment 8 Jesse Ruderman 2012-06-25 13:16:21 PDT
Now I'm getting a non-deterministic crash :(  I haven't tried Valgrind.
Comment 9 Jesse Ruderman 2012-07-09 15:30:12 PDT
Created attachment 640385 [details]
stack trace 1
Comment 10 Andrew McCreight [:mccr8] 2012-07-11 10:27:10 PDT
I'm guessing sec-crit on this because it is compartment mismatch.
Comment 11 Daniel Veditz [:dveditz] 2012-07-12 13:14:24 PDT
Do we know whether this affects older versions, like ESR or Firefox 14?
Comment 12 Andrew McCreight [:mccr8] 2012-07-12 13:55:25 PDT
I can take a crack at trying to reproduce this, as I'm mostly stalled out on my other XPConnect bugs.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-07-13 01:16:04 PDT
I had just managed to reproduce this reliable yesterday by rewinding my repo to rev 4d0aa137268b. I didn't get any farther than that though, so I'm happy for mccr8 to take a crack at it! :-)
Comment 14 Andrew McCreight [:mccr8] 2012-07-13 07:01:58 PDT
The newest version reproduces reliably on my local tree, which I updated last on July 3.

We're in some kind of CallJSNative, and it looks like we're crashing when we try to get the compartment of the return value.  I guess the next step is to figure out what code is actually being run in the native.
Comment 15 Andrew McCreight [:mccr8] 2012-07-13 11:08:21 PDT
So, this looks it might be a regression from bug 766430, but I can't be sure.  That landed a week before this bug was filed.

We're going into |obj_lookupGetter|, then hitting the |obj->isProxy()| case, then setting vp using |*vp = CastAsObjectJsval(desc.getter)|.  We return from lookupGetter, then attempt to examine the return value, and end up either crashing attempting to get the compartment of the return value, or we fail with a compartment mismatch, with a bogus looking compartment.

My guess is that desc.getter isn't a GC thing.  Either that or it was rooted improperly at some earlier point, but I'd think poisoning would cause use to die in a different way.  Probably Proxy::getPropertyDescriptor is doing something wrong, so I'll poke around in there.
Comment 16 Andrew McCreight [:mccr8] 2012-07-13 12:54:33 PDT
Jesse points out that I linked back to this bug.  I meant bug 762432.
Comment 17 Andrew McCreight [:mccr8] 2012-07-13 14:09:47 PDT
Created attachment 642039 [details] [diff] [review]
root moar, still needs a test

Regression from bug 762432.  The descriptor that was added to obj_lookupGetter isn't properly rooted.  As far as I can see, there's only a couple of lines where it should be unrooted (assuming that CallArgs is properly rooted somewhere up the line), so I'm not sure why this is as reproduceable as it is.  It makes me a little suspicious that this is the right fix, but it seems to work.
Comment 18 Andrew McCreight [:mccr8] 2012-07-14 07:49:35 PDT
There are a few other uses that should probably also be rooted:

- obj_lookupSetter (basically the same code as the getter)

- JS_GetPropertyAttrsGetterAndSetterById ( ).  I guess this should be changed?

- XrayWrapper<Base, Traits>::defineProperty ( ) seems to need it in the second use, but not the first.  The first use just checks if obj is null.
Comment 19 Andrew McCreight [:mccr8] 2012-07-15 06:14:06 PDT
Really, it seems to me that we should just kill off every use of PropertyDescriptor and JSPropertyDescriptor, except for arguments, where they function as a sort of already_addreffed<> kind of thing.  I'm not sure what the best way to enforce that is, though.  Probably out of scope for this particular bug, which I'd like to keep limited in scope.  Then I can file a public bug about cleaning up this class.
Comment 20 Andrew McCreight [:mccr8] 2012-07-15 14:21:21 PDT
Created attachment 642425 [details] [diff] [review]
this got a little out of hand, WIP
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-07-15 14:50:21 PDT
Ah, looks like this is the same issue as bug 766430. Hopefully luke r+es my patch over there. :-)
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-07-15 14:51:13 PDT
Or maybe this is the more correct fix? Let's see what luke thinks.
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-07-15 14:54:35 PDT
Doh! bug 767273. ;-)
Comment 24 Andrew McCreight [:mccr8] 2012-07-16 06:45:58 PDT
So, yeah, as per 767273 this is just a case of getPropertyDescriptor being called with an uninitialized struct, not a use-after-free.

I tried explicitly initializing PropertyDescriptor with 0xDEADBEEF, and you end up crashing with a compartment mismatch assertion.  When it is properly initialized, as happens in the constructor for the rooting descriptor, it is okay.
Comment 25 Andrew McCreight [:mccr8] 2012-07-16 06:50:33 PDT

*** This bug has been marked as a duplicate of bug 767273 ***
Comment 26 Andrew McCreight [:mccr8] 2012-07-31 06:55:11 PDT
Clearing the tracking flag because this is a dupe of another tracked (and fixed) bug.
Comment 27 Daniel Veditz [:dveditz] 2012-11-07 18:05:28 PST
Are these testcases different enough from bug 767273 that they're worth checking into the tree? If not please set the flag to in-testsuite- (minus).
Comment 28 Christian Holler (:decoder) 2013-01-04 18:42:44 PST
A testcase for this bug was already added in the original bug (bug 767273).

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