Closed
Bug 994466
Opened 11 years ago
Closed 10 years ago
CSP in C++: Fix memory leak in CSPContext
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
In the current implementation the Principal holds a reference to CSPContext and vice versa. We need to find a way to break that cycle. Probably best in nsDocument destructor
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mozilla
Assignee | ||
Comment 1•11 years ago
|
||
In an ideal world I would like to define nsDocument as a |friend| of nsCSPContext and not change the *.idl. Something like this:
> class nsCSPContext : public nsIContentSecurityPolicy
> {
> friend class nsDocument;
> friend void DropPrincipal() {
> mPrincipal = nullptr;
> }
> // class methods
> }
Unfortunately, the nsPrincipal holds a pointer to nsIContentSecurityPolicy which leaves no other option than extending the interface with |dropPrincipal|.
I haven't tested this closely, but this patch should fix the circular reference problem and therefore avoid the encountered memory leak.
A similar solution would drop the csp in the principal. But "Safety first"; worst case scenario with this the current patch is that we someone drops the Principal too early in nsIContentPolicy which would 'deny' reports to be sent in |SendReports|. So I guess dropping the principal in the contentPolicy is the better option.
Sid, what do you think?
Attachment #8404432 -
Flags: feedback?(sstamm)
Comment 2•11 years ago
|
||
I just spoke with :mccr8 and we should exhaust all possibility of using a weak reference from the CSP to Principal first. If that doesn't work, we might be able to use a mutation observer to detect events on the document (document destruction, for example) and break the cycle there, but that's not good if the document simply gets a new principal and is not destroyed.
So... lets try weak references to the principal first. Where will that not work, and can we get by without a principal (copy in relevant info) in those cases where the principal is destroyed before we use it?
Comment 3•11 years ago
|
||
Looks like the only place the principal is used by CSP is in the report sending logic. It's passed to NS_CheckContentLoadPolicy, and we might be able to get by without a principal, not sure.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> So... lets try weak references to the principal first. Where will that not
> work, and can we get by without a principal (copy in relevant info) in those
> cases where the principal is destroyed before we use it?
I am going to figure out in what cases the weakReference does not work. If the weak reference to the principal does not work, why not modify the current patch to use the following code in the destructor of nsDocument?
> if (principal) {
> nsCOMPtr<nsIContentSecurityPolicy> csp;
> if (NS_SUCCEEDED(principal->GetCsp(getter_AddRefs(csp))) && csp) {
> nsRefPtr<nsCSPContext> cspContext(do_QueryObject(csp));
> if (cspContext) {
> cspContext->DropPrincipal();
> }
> }
> }
No need to change the idl, and we would be sure that the document is destroyed, meaning the CSP is definitely not going to be needed again.
But in general, I agree, the weak reference to principal would be the best solution.
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> Looks like the only place the principal is used by CSP is in the report
> sending logic. It's passed to NS_CheckContentLoadPolicy, and we might be
> able to get by without a principal, not sure.
Test test/csp/test_csp_report.html times out in case the principal is null. Reason: NS_CheckContentLoadPolicy returns REJECT if no principal is provided.
Assignee | ||
Comment 6•11 years ago
|
||
Looks like we can call ShouldLoad directly instead of using the detour of calling NS_CheckContentLoadPolicy. Therefore we can use the selfURI as aRequestOrigin and can disregard the principal completely. Sid just pushed the fix to our csp patch queue.
Can we close this bug, or should we upload the patch here? Sid?
Assignee | ||
Comment 7•11 years ago
|
||
Instead of calling NS_CheckContentLoadPolicy we call ShouldLoad directly which allows us to skip the principal and use the originalURI directly. Awesome, easy fix for a memory leak after all.
Attachment #8404432 -
Attachment is obsolete: true
Attachment #8404432 -
Flags: feedback?(sstamm)
Attachment #8404883 -
Flags: review?(sstamm)
Attachment #8404883 -
Flags: review?(grobinson)
Comment 8•11 years ago
|
||
Comment on attachment 8404883 [details] [diff] [review]
bug_994466_change_call_to_shouldload.patch
Review of attachment 8404883 [details] [diff] [review]:
-----------------------------------------------------------------
yay for leak fixes! r=me if you fix the minors, and with garrett's review.
::: content/base/src/nsCSPContext.cpp
@@ +634,5 @@
>
> // refuse to load if we can't do a security check
> NS_ENSURE_SUCCESS(rv, rv);
>
> if (shouldLoad != nsIContentPolicy::ACCEPT) {
Please use NS_CP_REJECTED(shouldLoad) instead of !=.
@@ +637,5 @@
>
> if (shouldLoad != nsIContentPolicy::ACCEPT) {
> // skip unauthorized URIs
> // probably should report error
> return NS_OK;
This should probably be a "continue" because there may be other report URIs. And we should post a warning to the web console.
Attachment #8404883 -
Flags: review?(sstamm) → review+
Comment 9•11 years ago
|
||
The attached patch has some issues, but it appears to have been merged (and the issues fixed) in the latest commits from the patch queue. I'm clearing the review flag, if a review is still needed post an updated patch and re-flag me.
Updated•11 years ago
|
Attachment #8404883 -
Flags: review?(grobinson)
Comment 10•10 years ago
|
||
The patch here still needs an update to address the minors in comment 8 before it lands.
Assignee | ||
Comment 11•10 years ago
|
||
Waiting till other patches landed to avoid unnecessary exporting. Will address the nits in Comment 8 ASAP. Thanks for reminding!
Assignee | ||
Comment 12•10 years ago
|
||
Carrying over r+ from sstamm; also added him as a reviewer.
Please note that all the nits from comment 8 got incorporated into the patch in bug 994322 which has to land the at same time (but before) this patch.
Attachment #8404883 -
Attachment is obsolete: true
Attachment #8426352 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
I had this patch on try [1] together with the patch from bug 994322. Those two patches are ready to land and need to land together. First apply patch from bug 994322 and then this one on top. Thanks!
[1] https://tbpl.mozilla.org/?tree=Try&rev=955de4d3dda2
Keywords: checkin-needed
Comment 14•10 years ago
|
||
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•