Closed
Bug 246092
Opened 20 years ago
Closed 18 years ago
nsContentPolicy caches Category list
Categories
(Core :: Graphics: Image Blocking, defect)
Core
Graphics: Image Blocking
Tracking
()
RESOLVED
FIXED
People
(Reporter: timeless, Assigned: chpe)
References
Details
Attachments
(1 file, 6 obsolete files)
9.34 KB,
patch
|
timwatt
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•20 years ago
|
||
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.
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 5•20 years ago
|
||
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+
Assignee | ||
Comment 6•20 years ago
|
||
(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...
Assignee | ||
Comment 7•20 years ago
|
||
Attachment #171373 -
Attachment is obsolete: true
Comment 8•20 years ago
|
||
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.
Assignee | ||
Comment 9•19 years ago
|
||
(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)
Assignee | ||
Comment 10•19 years ago
|
||
And use a lock when accessing mPolicies.
Attachment #171658 -
Attachment is obsolete: true
Attachment #186726 -
Flags: review?(timwatt)
Comment 11•19 years ago
|
||
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.
Assignee | ||
Comment 12•19 years ago
|
||
Use nsAutoLock, and use a Init method for initialisation.
Assignee: security-bugs → chpe
Attachment #186726 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Updated•19 years ago
|
Attachment #186733 -
Flags: review?(timwatt)
Assignee | ||
Updated•19 years ago
|
Attachment #171373 -
Flags: review?(timwatt)
Assignee | ||
Updated•19 years ago
|
Attachment #186726 -
Flags: review?(timwatt)
Comment 13•19 years ago
|
||
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 14•19 years ago
|
||
Comment on attachment 186733 [details] [diff] [review] updated patch thanks to WeirdAl for helping me verify it.
Attachment #186733 -
Flags: review?(timwatt) → review+
Reporter | ||
Comment 15•19 years ago
|
||
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 @@
Assignee | ||
Comment 16•19 years ago
|
||
(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?
Reporter | ||
Comment 17•19 years ago
|
||
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.
Comment 18•19 years ago
|
||
> 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.
Assignee | ||
Comment 19•19 years ago
|
||
> 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?
Comment 20•19 years ago
|
||
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.
Reporter | ||
Comment 21•19 years ago
|
||
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.
Assignee | ||
Comment 22•19 years ago
|
||
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.
Comment 23•19 years ago
|
||
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.
Assignee | ||
Comment 24•19 years ago
|
||
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 25•19 years ago
|
||
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+
Assignee | ||
Comment 26•19 years ago
|
||
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 27•19 years ago
|
||
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+
Comment 28•19 years ago
|
||
we use static PRLogModule *gSuchandSuchLog = PR_NewLogModule(); all over the code, I think we should treat it as an acceptable pattern
Assignee | ||
Comment 29•18 years ago
|
||
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 30•18 years ago
|
||
Comment on attachment 214081 [details] [diff] [review] new patch I wonder if nsCategoryCache should have a Count() and operator[] function...
Comment 31•18 years ago
|
||
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+
Reporter | ||
Comment 32•18 years ago
|
||
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
Comment 33•18 years ago
|
||
So, is this fixed, and just not marked fixed?
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•