Closed
Bug 1072817
Opened 8 years ago
Closed 8 years ago
Crash with Proxy.revocable
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jruderman, Assigned: efaust)
Details
(Keywords: crash, csectype-nullptr, testcase)
Attachments
(4 files)
215 bytes,
text/html
|
Details | |
11.03 KB,
text/plain
|
Details | |
3.72 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
4.04 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a4ac30f999
Updated•8 years ago
|
Group: core-security
Keywords: sec-low → csectype-nullptr
https://hg.mozilla.org/mozilla-central/rev/f5a4ac30f999
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 9•8 years ago
|
||
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 → ---
Comment 10•8 years ago
|
||
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: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c795042b7dcd
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•