Closed Bug 494445 Opened 15 years ago Closed 15 years ago

MirrorWrappedNativeParent result sometimes uninitialized [@ js_LockGCThingRT ]


(Core :: XPConnect, defect)

Not set





(Reporter: peterv, Assigned: peterv)



(Keywords: crash, fixed1.9.0.12, fixed1.9.1, Whiteboard: [sg:critical?] required by 455633)

Crash Data


(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
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.
Flags: in-testsuite?
Flags: blocking1.9.1?
Attachment #379193 - Flags: superreview?(jst)
Attachment #379193 - Flags: review?(jst)
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.
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #379193 - Flags: superreview?(jst)
Attachment #379193 - Flags: superreview+
Attachment #379193 - Flags: review?(jst)
Attachment #379193 - Flags: review+
Comment on attachment 379193 [details] [diff] [review]

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.
Attached patch v1.1Splinter Review
With NS_OUTPARAM for static analysis goodness. Carrying forward r/sr=mrbkap.
Attachment #379193 - Attachment is obsolete: true
Attachment #379226 - Flags: superreview+
Attachment #379226 - Flags: review+
BTW, this is not a problem on older branches (but would be if bug 455633 is landed there).
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.1
Keywords: testcase-wanted
Whiteboard: [sg:critical?] required by 455633
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.
Flags: wanted1.9.0.x?
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).
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12+
mrbkap landed this fix with the fix for bug 455633 on the 1.9.0 branch.
Keywords: fixed1.9.0.12
Group: core-security
Crash Signature: [@ js_LockGCThingRT ]
You need to log in before you can comment on or make changes to this bug.