Crash with Proxy.revocable

RESOLVED FIXED in mozilla36

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jruderman, Assigned: efaust)

Tracking

(Blocks 1 bug, {crash, csectype-nullptr, testcase})

Trunk
mozilla36
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

No description provided.
Posted file stack
It looks like a proxy gets revoked and then we hit a null deref when we look at the proxy, so that doesn't sound too bad.
Keywords: sec-low
NI from our proxy guru :)
Flags: needinfo?(efaustbmo)
Posted patch FixSplinter Review
Don't let DirectProxyHandler come in contact with revoked proxies. Instead, add the needed-but-not-yet-written shells of get/set prototype of hooks.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8505052 - Flags: review?(jorendorff)
Flags: needinfo?(efaustbmo)
Comment on attachment 8505052 [details] [diff] [review]
Fix

Review of attachment 8505052 [details] [diff] [review]:
-----------------------------------------------------------------

Eric, while I was shuffling proxy methods over in bug 1081255, I noticed that some BaseProxyHandler subclasses don't have methods that I think they ought to have. This isn't the only case.

For example, it seems like DirectProxyHandler ought to override everything or nearly everything. It appears to be missing defaultValue, isConstructor, watch/unwatch, and slice. Maybe it's OK to omit slice. But what about the others?
Attachment #8505052 - Flags: review?(jorendorff) → review+
Posted patch Alterna-patchSplinter Review
So I independently-ish found this yesterday, as commented in bug 1052139 comment 9, and wrote a patch.  I also came to the conclusion that SDPH must override every single method in DPH, but before I could modify the patch to do that, I was pointed at this bug.  (Shows how much I'm reading bugmail these days, I guess.)

This patch is largely the same as the one posted here, just with a better (clearer, not implicating all the random trapping instanceof would) test in it.  May or may not be useful, but might as well post it since it's done.

Incidentally, I need these traps added before I can progress with bug 1052139, which adds a trap that'll go right next to these new ones in SDPH.

This is pretty much just going to be null-derefs, so I'm not sure why it's security-sensitive.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/f5a4ac30f999
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Hi Waldo,

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=accd28c1aaa3 since one of this changes/bugs caused perma failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3198627&repo=mozilla-inbound

btw there seems to be a checkin comment missing for https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecf4e364d05 because that was the one that got backed out, so reopening just in case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded the backed-out bit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c795042b7dcd

That was  just a very minor bit of method-reordering, plus my test, so no actual bug-fixing.  So back to FIXED it goes now.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.