MirrorWrappedNativeParent result sometimes uninitialized [@ js_LockGCThingRT ]

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

({crash, fixed1.9.0.12, fixed1.9.1})

Trunk
mozilla1.9.2a1
crash, fixed1.9.0.12, fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] required by 455633, crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
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+

Updated

8 years ago
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]
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.
(Assignee)

Comment 3

8 years ago
Created attachment 379226 [details] [diff] [review]
v1.1

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+
(Assignee)

Comment 4

8 years ago
BTW, this is not a problem on older branches (but would be if bug 455633 is landed there).
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
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 ]
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.