Last Comment Bug 494445 - MirrorWrappedNativeParent result sometimes uninitialized [@ js_LockGCThingRT ]
: MirrorWrappedNativeParent result sometimes uninitialized [@ js_LockGCThingRT ]
Status: RESOLVED FIXED
[sg:critical?] required by 455633
: crash, fixed1.9.0.12, fixed1.9.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9.2a1
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 455633
  Show dependency treegraph
 
Reported: 2009-05-22 11:16 PDT by Peter Van der Beken [:peterv]
Modified: 2015-10-16 11:48 PDT (History)
8 users (show)
dsicore: blocking1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
peterv: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (583 bytes, patch)
2009-05-22 11:16 PDT, Peter Van der Beken [:peterv]
mrbkap: review+
mrbkap: superreview+
Details | Diff | Splinter Review
v1.1 (887 bytes, patch)
2009-05-22 13:22 PDT, Peter Van der Beken [:peterv]
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2009-05-22 11:16:05 PDT
Created attachment 379193 [details] [diff] [review]
v1

I was looking at crash reports, there's a number of crashes in js_LockGCThingRT being called from XPCNativeWrapper::GetNewOrUsed. We call js_LockGCThingRT on the result of a call to MirrorWrappedNativeParent. Turns out that MirrorWrappedNativeParent sometimes returns PR_TRUE without initializing the result out pointer. I think setting it to null would be the right thing to do.

I think that this probably shows up under a number of different crash signatures, because XPCNativeWrapperCtor also calls MirrorWrappedNativeParent and it uses the uninitialized pointer to set it as a parent by calling JS_SetParent.

Nominating for blocking and marking security sensitive.
Comment 1 Damon Sicore (:damons) 2009-05-22 11:41:07 PDT
Talked this over with peterv a bit.  It's a nasty crash that results in an uninitialized pointer in the JS heap.  Since we have a pretty simple patch, and it's a security issue, I say we block and get this in asap.  If it comes down the the wire (*cough* may be there already) we can move on without this, but it doesn't seem to make sense to hold this back.
Comment 2 Blake Kaplan (:mrbkap) 2009-05-22 13:02:57 PDT
Comment on attachment 379193 [details] [diff] [review]
v1

I spent a while tracking down why we do this mirroring at all (it isn't clear what we're protecting with it) and the result I came up with was that this patch makes things no worse than they are now wrt XPCNativeWrappers' parents.
Comment 3 Peter Van der Beken [:peterv] 2009-05-22 13:22:15 PDT
Created attachment 379226 [details] [diff] [review]
v1.1

With NS_OUTPARAM for static analysis goodness. Carrying forward r/sr=mrbkap.
Comment 4 Peter Van der Beken [:peterv] 2009-05-22 15:01:31 PDT
BTW, this is not a problem on older branches (but would be if bug 455633 is landed there).
Comment 5 Daniel Veditz [:dveditz] 2009-05-25 20:08:36 PDT
setting wanted1.9.0.x? flag just so we don't keep triaging it when we go through trunk-fixed security bugs we haven't evaluated for the 1.9.0 branch. Better than a minus, which is true at the moment but might cause us to ignore this bug if bug 455633 is ever backported.
Comment 6 Daniel Veditz [:dveditz] 2009-07-05 22:46:17 PDT
We are landing bug 455633 on the 1.9.0 branch to fix regression bug 502458, therefore we need this fix too (rolled into the branch patch in bug 455633).
Comment 7 Daniel Veditz [:dveditz] 2009-07-05 23:09:27 PDT
mrbkap landed this fix with the fix for bug 455633 on the 1.9.0 branch.

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