Closed Bug 751858 Opened 8 years ago Closed 8 years ago

location's PUNCTURE policy throws without setting an exception

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(1 file, 1 obsolete file)

I don't have a testcase right now, but I ran into this working on another bug. Basically, Policy::check functions are responsible for setting their own exceptions if they deny access.
Attached patch Proposed fix (obsolete) — Splinter Review
I think the change to AccessCheck.cpp makes the control flow a little clearer (and it nukes a couple of braces). It's a little unfortunate that this patch actually makes LocationPolicy::check less clear (since the exception for PUNCTURE is "hidden" in a larger boolean expression) but it's the easiest way to share the code that throws the exception.
Attachment #621016 - Flags: review?(bobbyholley+bmo)
Comment on attachment 621016 [details] [diff] [review]
Proposed fix


>--- a/js/xpconnect/wrappers/AccessCheck.cpp
>+++ b/js/xpconnect/wrappers/AccessCheck.cpp
>@@ -494,12 +494,10 @@ ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper:
>         perm = PermitObjectAccess;
>         return true;
>     }
>-    if (act == Wrapper::PUNCTURE) {
>-        perm = DenyAccess;
>-        return false;
>-    }
> 
>     perm = DenyAccess;
>+    if (act == Wrapper::PUNCTURE)
>+        return false;

Doesn't this have the same issue?
Attached patch Proposed fix v2Splinter Review
Oops, yes.
Attachment #621016 - Attachment is obsolete: true
Attachment #621016 - Flags: review?(bobbyholley+bmo)
Attachment #621024 - Flags: review?(bobbyholley+bmo)
Comment on attachment 621024 [details] [diff] [review]
Proposed fix v2

Can we add some tests for this? I'm about to touch this code in a couple of ways (fixing __exposedProps__, and removing universalXPConnect), and this seems like the sort of thing I might regress.
Attachment #621024 - Flags: review?(bobbyholley+bmo) → review+
I think so, yes.
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/8d752d1b621b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.