Closed Bug 766430 Opened 10 years ago Closed 10 years ago

Uninitialized local variable with childNodes, lookupGetter

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 767273
Tracking Status
firefox13 --- unaffected
firefox14 --- unaffected
firefox15 --- unaffected
firefox16 --- fixed
firefox17 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: jruderman, Assigned: mccr8)

References

Details

(4 keywords, Whiteboard: [advisory-tracking-][sg:dupe 767273])

Attachments

(6 files, 1 obsolete file)

Attached file testcase (asserts fatally when loaded) (obsolete) —
No description provided.
Group: core-security
I can't reproduce this on my trunk build (debug, macosx64). Jesse, any chance this was fixed by bug 764289?
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?
Attached file updated testcase
I had to flesh the testcase out a little bit to get it to crash. Updating.
Attachment #634712 - Attachment is obsolete: true
And now I need a setTimeout to reproduce :/
(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.
Now I'm getting a non-deterministic crash :(  I haven't tried Valgrind.
Attached file stack trace 1
I'm guessing sec-crit on this because it is compartment mismatch.
Keywords: sec-critical
Do we know whether this affects older versions, like ESR or Firefox 14?
I can take a crack at trying to reproduce this, as I'm mostly stalled out on my other XPConnect bugs.
Assignee: nobody → continuation
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! :-)
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.
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.
Jesse points out that I linked back to this bug.  I meant bug 762432.
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.
Component: DOM → JavaScript Engine
Summary: Compartment mismatch with childNodes, lookupGetter → Use-after-free with childNodes, lookupGetter
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.
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.
Ah, looks like this is the same issue as bug 766430. Hopefully luke r+es my patch over there. :-)
Or maybe this is the more correct fix? Let's see what luke thinks.
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.
Summary: Use-after-free with childNodes, lookupGetter → Uninitialized local variable with childNodes, lookupGetter
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 767273
Clearing the tracking flag because this is a dupe of another tracked (and fixed) bug.
Whiteboard: [advisory-tracking-]
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).
Group: core-security
Flags: in-testsuite?
Whiteboard: [advisory-tracking-] → [advisory-tracking-][sg:dupe 767273]
A testcase for this bug was already added in the original bug (bug 767273).
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.