Closed Bug 246092 Opened 20 years ago Closed 17 years ago

nsContentPolicy caches Category list

Categories

(Core :: Graphics: Image Blocking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: chpe)

References

Details

Attachments

(1 file, 6 obsolete files)

Categories can change. nsContentPolicy should either listen to a hypothetical
obserer notification for category additions/removals (bug 246085), or get the
current category list before doing work.
-> default owner, until I get some time to give this attention.
Assignee: riceman+bmo → security-bugs
This patch updates the list of content policies in mPolicies when the
content-policy category changes; needs the patch from bug 246085 (attachment
171372 [details] [diff] [review]) to work.
Depends on: 246085
Comment on attachment 171373 [details] [diff] [review]
update mPolicies on category change

>Index: content/base/src/nsContentPolicy.cpp
>@@ -22,136 +22,169 @@
...
>+    UpdatePolicies();
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIObserverService> observerService
>+        = do_GetService("@mozilla.org/observer-service;1", &rv);
>+    if (NS_FAILED(rv))
since you don't use rv past here, you can use the one arg form of do_GetService
and null check observerService instead of storing rv.
>+        return;
>+    observerService->AddObserver(this, "category-changed", PR_TRUE);
>+}

do you want to use the define from category manager?
>+    if (!strcmp (aTopic, "category-changed")) {
Attachment #171373 - Flags: superreview?(bzbarsky)
Attachment #171373 - Flags: review?(timwatt)
The code looks reasonable, given a couple assumptions (which I'll state, since
my memory of Mozilla threading is fuzzy).

Prerequisites for this patch being adequate:
1. All observer notifications occur in a serial fashion (we won't ever get
notified a second time while we are processing a notification).
2. All accesses to Content Policy are more or less serial (since mPolicies is in
an indeterminate state for the duration of UpdatePolicies, CheckPolicy must not
be run until UpdatePolicies finishes).

For example, if some somebody screws with the list while a web page is loading,
can we be sure that CheckPolicy will not overlap with UpdatePolicies?  If not,
this patch will not work (for that situation, anyway).

I'll hold my review until someone comments (in this bug or privately to me)
about the above.
Comment on attachment 171373 [details] [diff] [review]
update mPolicies on category change

>Index: content/base/src/nsContentPolicy.cpp
>+    observerService->AddObserver(this, "category-changed", 

Don't we need to RemoveObserver() even if we're being held as weak?  

>+nsContentPolicy::Observe(nsISupports     *aSubject,
>+    if (!strcmp (aTopic, "category-changed")) {


Kill space after "strcmp".

>+        nsAutoString categoryName (someData);

nsDependentString, please.

>Index: content/base/src/nsContentPolicy.h
>+class nsContentPolicy : public nsIContentPolicy,
>+			public nsIObserver,
>+			public nsSupportsWeakReference

Fix the indent here.

sr=bzbarsky with those nits fixed.
Attachment #171373 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #5)
> (From update of attachment 171373 [details] [diff] [review] [edit])
> >Index: content/base/src/nsContentPolicy.cpp
> >+    observerService->AddObserver(this, "category-changed", 
> 
> Don't we need to RemoveObserver() even if we're being held as weak?  

Not sure where I'd place the RemoveObserver() call; if I do
do_GetService("@mozilla.org/observer-service;1"); in ~nsContentPolicy, it
returns null...

> >+nsContentPolicy::Observe(nsISupports     *aSubject,
> >+    if (!strcmp (aTopic, "category-changed")) {
> 
> Kill space after "strcmp".
Done.

> >+        nsAutoString categoryName (someData);
> 
> nsDependentString, please.
Fixed.
 
> >Index: content/base/src/nsContentPolicy.h
> >+class nsContentPolicy : public nsIContentPolicy,
> >+			public nsIObserver,
> >+			public nsSupportsWeakReference
> 
> Fix the indent here.
Fixed.

Attaching updated patch...

Attached patch updated patch (obsolete) — Splinter Review
Attachment #171373 - Attachment is obsolete: true
Calling RemoveObserver is not really necessary if this observer is meant to
exist for the lifetime of the app.  NS_ShutdownXPCOM causes the observer service
to remove any remaining observers after the xpcom-shutdown notification has been
dispatched.
(In reply to comment #4)
> The code looks reasonable, given a couple assumptions (which I'll state, since
> my memory of Mozilla threading is fuzzy).
> 
> Prerequisites for this patch being adequate:
> 1. All observer notifications occur in a serial fashion (we won't ever get
> notified a second time while we are processing a notification).
> 2. All accesses to Content Policy are more or less serial (since mPolicies is in
> an indeterminate state for the duration of UpdatePolicies, CheckPolicy must not
> be run until UpdatePolicies finishes).
> 
> For example, if some somebody screws with the list while a web page is loading,
> can we be sure that CheckPolicy will not overlap with UpdatePolicies?  If not,
> this patch will not work (for that situation, anyway).

The category could be changed from any thread (cat man is threadsafe). When the
category changes, the observer service's NotifyObservers is called on that
thread, and it notifies on the same thread. So, conceivably, there could be
another notification while one notification is being processed, on a different
thread, which would leave mPolicies in an undetermined state.

Solutions that I can think of are:
a) protect mPolicies with a lock, or
b) change the behaviour of the observer service wrt. threads: notifications
could be proxied to the thread the observer was added from (or maybe to the main
thread)
And use a lock when accessing mPolicies.
Attachment #171658 - Attachment is obsolete: true
Attachment #186726 - Flags: review?(timwatt)
Comment on attachment 186726 [details] [diff] [review]
updated patch, taking into account the checked-in patch from bug 246085

why not give the class an nsresult Init(); function which takes care of the
initialization?

Also, you could use nsAutoLock to ensure that all codepaths will unlock the
lock.
Attached patch updated patch (obsolete) — Splinter Review
Use nsAutoLock, and use a Init method for initialisation.
Assignee: security-bugs → chpe
Attachment #186726 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186733 - Flags: review?(timwatt)
Attachment #171373 - Flags: review?(timwatt)
Attachment #186726 - Flags: review?(timwatt)
Comment on attachment 186733 [details] [diff] [review]
updated patch

looks good, conditional on the following:

sr (or someone else) needs to verify that it compiles and runs

Keep an eye on Tp, since this will probably get called a bunch of times for
every page load. (Ts/Txul may also deserve watching if any of chrome gets
routed through content policy--I don't remember whether this is the case or
not.)

I'm going to dig up someone to verify it as mentioned before I + it.
Comment on attachment 186733 [details] [diff] [review]
updated patch

thanks to WeirdAl for helping me verify it.
Attachment #186733 - Flags: review?(timwatt) → review+
Comment on attachment 186733 [details] [diff] [review]
updated patch

>Index: nsContentPolicy.cpp
>@@ -14,152 +14,221 @@
> nsContentPolicy::nsContentPolicy()
you should init mLock(nsnull) here, otherwise someone who fails to call Init
will crash.

>+nsContentPolicy::Init()
>+    if (observerService) {
tabs:
>+	if (NS_FAILED(rv))
>+            return rv;

>@@ -171,58 +240,60 @@ nsContentPolicy::CheckPolicy(CPMethod   
>+    nsAutoLock lock(mLock);
>      * Enumerate mPolicies and ask each of them, taking the logical AND of
>      * their permissions.
>     for (PRInt32 i = 0; i < count; i++) {
calling a method while locked isn't very nice and is probably asking for
deadlock.
>         rv = (policy->*policyMethod)(contentType, contentLocation,
>                                      requestingLocation, requestingContext,
>                                      mimeType, extra, decision);

>Index: nsContentPolicy.h
>@@ -15,63 +15,73 @@
(In reply to comment #15)
> (From update of attachment 186733 [details] [diff] [review] [edit])
> >Index: nsContentPolicy.cpp
> >@@ -14,152 +14,221 @@
> > nsContentPolicy::nsContentPolicy()
> you should init mLock(nsnull) here, otherwise someone who fails to call Init
> will crash.
> >+nsContentPolicy::Init()
> >+    if (observerService) {
> tabs:
> >+	if (NS_FAILED(rv))
> >+            return rv;
Both fixed in my tree.
 
> >@@ -171,58 +240,60 @@ nsContentPolicy::CheckPolicy(CPMethod   
> >+    nsAutoLock lock(mLock);
> >      * Enumerate mPolicies and ask each of them, taking the logical AND of
> >      * their permissions.
> >     for (PRInt32 i = 0; i < count; i++) {
> calling a method while locked isn't very nice and is probably asking for
> deadlock.
> >         rv = (policy->*policyMethod)(contentType, contentLocation,
> >                                      requestingLocation, requestingContext,
> >                                      mimeType, extra, decision);

I don't understand: under what circumstances would a evaluating a policy lead to
re-entering nsContentPolicy::ShouldLoad or ::ShouldProcess? As far as I can see,
that would mean that the policy tries to load something in the page; but why
should we allow that?
i'm thinking more about a policy which decides during a shouldload call to
register for another xpcom category, which enters your observe method and gets
killed by nspr for locking twice.

there's a general rule: don't hold locks while calling outside your module. it
very much applies here.
> there's a general rule: don't hold locks while calling outside your module. it
> very much applies here.

ditto!  even so called re-entrant locks (monitors) are not safe.  you should
push whatever state you need onto the stack, then exit the lock, and then
finally issue the observer callback based only on the state stored on the stack.

However, I now have a question:

+nsresult
+nsContentPolicy::Init()
+{
+    mLock = PR_NewLock();
+    if (!mLock) {
+        return NS_ERROR_OUT_OF_MEMORY;
+    }
+

Why does this object need a lock?

OK, I see the comment about the observer service's NotifyObservers method being
run on the thread that changed a category.  That is just majorly broken.  The
observer service is not designed with multiple threads in mind.  The category
service is also not well designed if it is triggering observer notifications on
a background thread.  I don't think this code (nsContentPolicy) should have to
be designed to be poked on background threads.  Gecko (in this case the stuff
under content/) is not threadsafe -- nor should it be -- and we definitely don't
want to incrementally make it so without a high level design.  Let's re-assess
why we are locking here, and see if we can't eliminate that.
> However, I now have a question:
> 
> +nsresult
> +nsContentPolicy::Init()
> +{
> +    mLock = PR_NewLock();
> +    if (!mLock) {
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +
> 
> Why does this object need a lock?
> 
> OK, I see the comment about the observer service's NotifyObservers method being
> run on the thread that changed a category.  That is just majorly broken.  The
> observer service is not designed with multiple threads in mind.  The category
> service is also not well designed if it is triggering observer notifications on
> a background thread.  I don't think this code (nsContentPolicy) should have to
> be designed to be poked on background threads.  Gecko (in this case the stuff
> under content/) is not threadsafe -- nor should it be -- and we definitely don't
> want to incrementally make it so without a high level design.  Let's re-assess
> why we are locking here, and see if we can't eliminate that.

The observer service's NotifyObservers could be changed to call the observers'
::Observe on the main thread. Would that introduce too much of a performance
penalty for the most common case when NotifyObservers is already called on the
main thread?
Or alternatively and just for this bug, the category manager could make sure it
calls the observer service's NotifyObservers on the main thread.
Or the category manager could refuse category changes from other threads.

Which one would you prefer? Is there another way that I haven't found?
I'm not 100% sure, but I think it probably makes sense to dispatch the observer
notifications to the main thread.  I would document this in the API someplace. 
Using XPCOM proxies, you can basically call a function on the main thread and
have it build and use a proxy only if called from a background thread.  That
way, there is no cost if calling the function on the main thread (which is the
common case).

  NS_GetProxyForObject(..., PROXY_ASYNC, ...);

Provided you do not OR in PROXY_ALWAYS, you will get the behavior I described.
darin: speaking of broken our proxies don't support safely proxying a
threadunsafe object to the main thread such that it could participate in this
game, currently that the observer service pretends to be threadsafe at least
partially alleviates this mess.
I'm not sure if in comment 20 darin meant that all notifications ought to be
sent to main thread, or just the category change notifications. Here's a patch
that implements the latter.
It might make sense to proxy all observer service notifications to the main
thread, but that seems like a more involved change.  At this point, we probably
want to minimize the scope of the changes, so the suggested patch seems best to me.
I posted the patch for proxying the notifications to the main thread in bug
246085. Unless I'm misunderstanding how threads work, that should make it
impossible to get to GetPolicies while a ShouldLoad or ShouldProcess is being
processed, so no locks are needed anymore. That should address Tim's concerns
in comment 4.
Attachment #186733 - Attachment is obsolete: true
Attachment #188960 - Attachment is obsolete: true
Attachment #189594 - Flags: review?(timwatt)
Comment on attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

For future reference, it's handy when you make patches from mozilla/, but that
isn't really important.

It looks good, but hold it until bug 246085 gets that patch checked in (unless
someone thinks the other one will make it before the next alpha/beta/release of
the various tier-1 products).

It doesn't seem to break anything in seamonkey or firefox (today's CVS) by my
90-second "load-some-pages" testing. (Note I only tested it with the 246085
patch, but my understanding is that the problem this solves doesn't exist in
main-line anyway--that it only affects third-party extensions/embedders).

If anyone has a useful test case (extension, etc.) that verifies that this
really fixes the problem, speak up.
Attachment #189594 - Flags: review?(timwatt) → review+
Comment on attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

(In reply to comment #25)
> (From update of attachment 189594 [details] [diff] [review] [edit])
> It looks good, but hold it until bug 246085 gets that patch checked in 
That patch has been checked in.

Asking for sr now.
Attachment #189594 - Flags: superreview?(bzbarsky)
Comment on attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

>Index: nsContentPolicy.cpp
> nsContentPolicy::nsContentPolicy()
> {
> #ifdef PR_LOGGING
>     if (! gConPolLog) {
>         gConPolLog = PR_NewLogModule("nsContentPolicy");

That can fail, no?  This should probably live in Init() as well.

sr=bzbarsky with that addressed.
Attachment #189594 - Flags: superreview?(bzbarsky) → superreview+
we use static PRLogModule *gSuchandSuchLog = PR_NewLogModule(); all over the code, I think we should treat it as an acceptable pattern
Attached patch new patch (obsolete) — Splinter Review
I've gone back and ripped out most of my new code, since I can now simply use a nsCategoryCache<nsIContentPolicy> which keeps the list in sync with the category for me.

I didn't move the PR_NewLogModule to Init since I removed Init again; also if this is really a problem it should be done in a new bug (since this pattern is commonly used in many places).
Attachment #214081 - Flags: superreview?(bzbarsky)
Attachment #214081 - Flags: review?(timwatt)
Comment on attachment 214081 [details] [diff] [review]
new patch

I wonder if nsCategoryCache should have a Count() and operator[] function...
Comment on attachment 214081 [details] [diff] [review]
new patch

r+sr=bzbarsky
Attachment #214081 - Flags: superreview?(bzbarsky)
Attachment #214081 - Flags: superreview+
Attachment #214081 - Flags: review?(timwatt)
Attachment #214081 - Flags: review+
Comment on attachment 214081 [details] [diff] [review]
new patch

mozilla/content/base/src/nsContentPolicy.h 	3.10
mozilla/content/base/src/nsContentPolicy.cpp 	3.16
mozilla/content/base/src/nsContentPolicy.cpp 	3.17
Attachment #214081 - Attachment is obsolete: true
So, is this fixed, and just not marked fixed?
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: