494445 - MirrorWrappedNativeParent result sometimes uninitialized [@ js_LockGCThingRT ]

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

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

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

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

Attachment: v1.1

With NS_OUTPARAM for static analysis goodness. Carrying forward r/sr=mrbkap.

Comment 4

[Peter Van der Beken [:peterv]]

BTW, this is not a problem on older branches (but would be if bug 455633 is landed there).

Comment 5

[Daniel Veditz [:dveditz]]

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

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

mrbkap landed this fix with the fix for bug 455633 on the 1.9.0 branch.