766430 - Uninitialized local variable with childNodes, lookupGetter

Status: RESOLVED DUPLICATE of bug 767273

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Comment 1

[Bobby Holley (:bholley)]

I can't reproduce this on my trunk build (debug, macosx64). Jesse, any chance this was fixed by bug 764289?

Comment 2

[Jesse Ruderman]

Hrm, it still asserts/crashes for me, with a blank profile, both with https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1340204295/ and with a local build (mozilla-central-97119-e240d6e43c9a-debug).

Do you want to debug on my computer?

Comment 3

[Bobby Holley (:bholley)]

Attachment: updated testcase

I had to flesh the testcase out a little bit to get it to crash. Updating.

Comment 4

[Jesse Ruderman]

And now I need a setTimeout to reproduce :/

Comment 5

[Bobby Holley (:bholley)]

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

Attachment: testcase with setTimeout

Comment 7

[Jesse Ruderman]

Attachment: testcase that tries harder

This one requires https://www.squarefree.com/extensions/domFuzzLite3.xpi

Comment 8

[Jesse Ruderman]

Now I'm getting a non-deterministic crash :(  I haven't tried Valgrind.

Comment 9

[Jesse Ruderman]

Attachment: stack trace 1

Comment 10

[Andrew McCreight [:mccr8]]

I'm guessing sec-crit on this because it is compartment mismatch.

Comment 11

[Daniel Veditz [:dveditz]]

Do we know whether this affects older versions, like ESR or Firefox 14?

Comment 12

[Andrew McCreight [:mccr8]]

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

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

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

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

Jesse points out that I linked back to this bug.  I meant bug 762432.

Comment 17

[Andrew McCreight [:mccr8]]

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

There are a few other uses that should probably also be rooted:

- obj_lookupSetter (basically the same code as the getter)

- JS_GetPropertyAttrsGetterAndSetterById ( http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#3967 ).  I guess this should be changed?

- XrayWrapper<Base, Traits>::defineProperty ( http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#1417 ) 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]]

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

Attachment: this got a little out of hand, WIP

Comment 21

[Bobby Holley (:bholley)]

Ah, looks like this is the same issue as bug 766430. Hopefully luke r+es my patch over there. :-)

Comment 22

[Bobby Holley (:bholley)]

Or maybe this is the more correct fix? Let's see what luke thinks.

Comment 23

[Bobby Holley (:bholley)]

Doh! bug 767273. ;-)

Comment 24

[Andrew McCreight [:mccr8]]

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 26

[Andrew McCreight [:mccr8]]

Clearing the tracking flag because this is a dupe of another tracked (and fixed) bug.

Comment 27

[Daniel Veditz [:dveditz]]

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

A testcase for this bug was already added in the original bug (bug 767273).