Closed
Bug 1156875
Opened 9 years ago
Closed 9 years ago
URL.createObjectURL leaks in JS sandbox
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mt, Assigned: baku)
References
Details
Attachments
(3 files, 7 obsolete files)
29.32 KB,
patch
|
Details | Diff | Splinter Review | |
15.89 KB,
patch
|
Details | Diff | Splinter Review | |
1023 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Unless you call URL.revokeObjectURL(), URL.createObjectURL() leaks when the sandbox is destroyed.
Assignee | ||
Comment 1•9 years ago
|
||
Can you provide a small test case? This would be helpful!
Flags: needinfo?(martin.thomson)
Reporter | ||
Comment 2•9 years ago
|
||
Check out the patches on bug 1155898. Remove the URL.revokeObjectURL() call that I added to https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic.js
Flags: needinfo?(martin.thomson)
Reporter | ||
Comment 3•9 years ago
|
||
Otherwise, here's one you can run in an xpcshell test. var sb = new Cu.Sandbox('https://example.com', { wantGlobalProperties: [ 'Blob', 'URL' ] }); Cu.evalInSandbox('var u = URL.createObjectURL(new Blob(["text"], { type: "text/plain" }));', sb); Cu.nukeSandbox(sb);
Assignee | ||
Comment 4•9 years ago
|
||
bz, I wonder if we have a 'global' object where we can store the list of blob URIs from a sandbox. We have a similar approach in WorkerPrivate and in nsGlobalWindow.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 5•9 years ago
|
||
baku, any reason that you can't just use SandboxPrivate?
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8597943 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → amarchesini
Comment 8•9 years ago
|
||
Could we possibly just put this on nsIGlobalObject so that it will happen more automatically?
Reporter | ||
Comment 9•9 years ago
|
||
I thought about that; it would definitely save on special-case code. Maybe use a subordinate object on nsIGlobalObject so that the only cost to most globals is a (null) pointer and instantiate it as needed.
Comment 10•9 years ago
|
||
This patch uses nsTArray<nsCString> mHostObjectURIs. That has the same size as a pointer until you put something in the array.
Comment 11•9 years ago
|
||
Yeah, making this automatic somehow would be great. Is somebody in this bug willing to do that?
Assignee | ||
Updated•9 years ago
|
Attachment #8597943 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 12•9 years ago
|
||
I'm doing it.
Assignee | ||
Comment 13•9 years ago
|
||
bholley, do you want to review this patch?
Attachment #8597943 -
Attachment is obsolete: true
Attachment #8598229 -
Flags: review?(bobbyholley)
Comment 14•9 years ago
|
||
Comment on attachment 8598229 [details] [diff] [review] global.patch Review of attachment 8598229 [details] [diff] [review]: ----------------------------------------------------------------- r=me modulo that I guess, though I really wish we had a way to assert that we always traversed/unlinked this stuff. ::: dom/base/nsIGlobalObject.cpp @@ +99,5 @@ > + if (mHostObjectURIs.IsEmpty()) { > + return; > + } > + > + // Currently we only store FileImpl objects out of the main-thread and they s/out of the/off the/ @@ +101,5 @@ > + } > + > + // Currently we only store FileImpl objects out of the main-thread and they > + // are not CCed. > + if (NS_IsMainThread()) { Err, don't you mean _!_NS_IsMainThread() here? IIUC, this means that the traversal basically never runs in the current patch. Please find a way to add test coverage that catches this. ::: dom/base/nsIGlobalObject.h @@ +40,5 @@ > + > +protected: > + virtual ~nsIGlobalObject(); > + > + nsTArray<nsCString> mHostObjectURIs; Can we make this private? ::: dom/base/test/test_sandboxed_blob_uri.html @@ +15,5 @@ > + var sb = new Cu.Sandbox('https://example.com', { wantGlobalProperties: [ 'Blob', 'URL' ] }); > + Cu.evalInSandbox('var u = URL.createObjectURL(new Blob(["text"], { type: "text/plain" }));', sb); > + Cu.nukeSandbox(sb); > + > + ok(true, "are we leaking blobs?"); Maybe force a GC here? ::: js/xpconnect/src/Sandbox.cpp @@ +57,5 @@ > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(SandboxPrivate) > + > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS Nit - why the whitespace above?
Attachment #8598229 -
Flags: review?(bobbyholley) → review+
Comment 15•9 years ago
|
||
Andrew, do you have any idea how we can assert that these traverse/unlink methods are always called when cycle-collecting one of the subclasses, or make it less error-prone somehow?
Flags: needinfo?(continuation)
Comment 16•9 years ago
|
||
Inheriting from multiple cycle collected classes hasn't been a very common situation so far, so we don't really have anything to help protect against that. I'm not sure what you can do, short of static analysis.
Flags: needinfo?(continuation)
Assignee | ||
Comment 17•9 years ago
|
||
> IIUC, this means that the traversal basically never runs in the current
> patch. Please find a way to add test coverage that catches this.
Actually, it was quite orange on treeherder.
Assignee | ||
Comment 18•9 years ago
|
||
Here a second patch that unifies the WorkerPrivate with the nsIGlobalObject. I also removed the assertions because I realized that we should not crash if the user does: URL.revokeObjectURL('blob:something_fake');
Attachment #8598545 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 19•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1307ad7b37c
Comment 20•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #15) > Andrew, do you have any idea how we can assert that these traverse/unlink > methods are always called when cycle-collecting one of the subclasses, or > make it less error-prone somehow? Hmm, should we perhaps somehow just prevent inhering from multiple CCed classes. I think so.
Comment 21•9 years ago
|
||
So in this case, if nsGlobalWindow inherits from both EventTarget and nsIGlobalObject, and we want to put some CCed stuff in nsIGlobalObject... how should that work?
Comment 22•9 years ago
|
||
oh, that way. Then just add Traverse/Unlink methods explicitly to nsIGlobalObject and call them always in child. Error prone sure. Looks like the patch does effectively that. (I'd try to make all the globals EventTargets, and then have EventTarget>nsIGlobalObject>nsGlobalWindow class hierarchy. Would bring some more consistency to globals. But not in this bug.)
Reporter | ||
Comment 23•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #18) > I also removed the assertions because I realized that we should not crash if > the user does: URL.revokeObjectURL('blob:something_fake'); Is there a test for that somewhere? Might pay to have one.
Assignee | ||
Comment 24•9 years ago
|
||
mochitest added.
Attachment #8598545 -
Attachment is obsolete: true
Attachment #8598545 -
Flags: review?(bobbyholley)
Attachment #8598717 -
Flags: review?(bobbyholley)
Reporter | ||
Updated•9 years ago
|
Attachment #8598717 -
Flags: feedback+
Comment 25•9 years ago
|
||
Comment on attachment 8598717 [details] [diff] [review] global2.patch Review of attachment 8598717 [details] [diff] [review]: ----------------------------------------------------------------- This whole aToBeRegistered thing seems super error-prone. Why do we need this inconsistent behavior here? ::: dom/base/nsIGlobalObject.cpp @@ -24,5 @@ > > void > nsIGlobalObject::RegisterHostObjectURI(const nsACString& aURI) > { > - MOZ_ASSERT(!mHostObjectURIs.Contains(aURI)); Hm, shouldn't we keep this assertion? It seems like we don't really handle duplicates here.
Attachment #8598717 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 26•9 years ago
|
||
> This whole aToBeRegistered thing seems super error-prone. Why do we need
> this inconsistent behavior here?
Because you can create a worker, in the worker you create a blob and a blob URL. You send the blob URL to the main-thread. Then the worker is CCed. You want to have that blob URL still alive.
This is the reason why, if we have a parent window, the blob URLs are registered in that parent nsIGlobalObject.
Otherwise, if we don't have a window, we are in a 'special worker' setup: SharedWorkers, ServiceWorkers, etc. For those, we want to do the auto-revoke when they go away. So the blob URIs are registered in the nsIGlobalObject of the worker.
Does this answer your question?
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8598717 [details] [diff] [review] global2.patch I'm going to reintroduce the first assertion.
Attachment #8598717 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 28•9 years ago
|
||
Better approach without bool*.
Attachment #8598717 -
Attachment is obsolete: true
Attachment #8598717 -
Flags: review?(bobbyholley)
Attachment #8598821 -
Flags: review?(bobbyholley)
Comment 29•9 years ago
|
||
Comment on attachment 8598821 [details] [diff] [review] global2.patch Review of attachment 8598821 [details] [diff] [review]: ----------------------------------------------------------------- Hm, it seems kind of brittle that our policy here depends on whether or not we have a Window (what about workers-in-sandboxes etc?), but that's out-of-scope for this bug. I do think that rather than stashing this on the runnable, we should stash something on the URL object called "mRegisteredWithMainThreadGlobal". This gets computed during the creation runnable, and never changes. All subsequent registration decisions and deregistrations decisions would be made based on that. Does that make sense? Also the explanation in comment 26 should go into a comment somewhere here.
Attachment #8598821 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 30•9 years ago
|
||
> made based on that. Does that make sense?
URL.createObjectURL and removeObjectURL are static methods. We don't have any URL object around. In JS you do:
var blob = new Blob([123]);
var url = URL.createObjectURL(blob);
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bobbyholley)
Comment 31•9 years ago
|
||
Comment on attachment 8598821 [details] [diff] [review] global2.patch Review of attachment 8598821 [details] [diff] [review]: ----------------------------------------------------------------- Oh I see. Ok, I guess in that case just rename the methods to {Unr,R}egisteredWithMainThreadGlobal (and invert their semantics), and add the comment explaining what's going on here.
Attachment #8598821 -
Flags: review+
Updated•9 years ago
|
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 32•9 years ago
|
||
This patch uses the nsIGlobalObject of the parent of the top-level worker. This means that also for a worker created in a sandbox, we use the nsIGlobalObject of the sandbox to keep alive the blob URLs. For SharedWorkers and ServiceWorkers only we do the registration in the WorkerPrivate. I spoke with ehsan about if it's correct to do the registration in the WorkerPrivate also for ServiceWorkers and, we filed a spec bug, because this is unclear. In case, the changes will be implemented in a follow-up.
Attachment #8598821 -
Attachment is obsolete: true
Attachment #8598988 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #8598988 -
Attachment is obsolete: true
Attachment #8598988 -
Flags: review?(bent.mozilla)
Attachment #8599512 -
Flags: review?(bent.mozilla)
Comment on attachment 8599512 [details] [diff] [review] global2.patch Review of attachment 8599512 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these fixed: ::: dom/workers/URL.cpp @@ +78,5 @@ > nsAString& aURL) > + : WorkerMainThreadRunnable(aWorkerPrivate) > + , mBlobImpl(aBlobImpl) > + , mURL(aURL) > + , mRegisteredWithMainThreadGlobal(true) Hm, you know here whether or not aWorkerPrivate is a shared/service worker. Why wait to set this on the main thread? I think you could just get rid of this member and always do the special scope register/unregister for shared/service workers. Same for RevokeURLRunnable. @@ +142,5 @@ > + // Walk up to top worker object. > + WorkerPrivate* wp = mWorkerPrivate; > + while (wp->GetParent()) { > + wp = wp->GetParent(); > + } Nit: while (WorkerPrivate* parent = wp->GetParent()) { wp = parent; } @@ +145,5 @@ > + wp = wp->GetParent(); > + } > + > + nsCOMPtr<nsIScriptContext> sc = wp->GetScriptContext(); > + if (sc) { You need to comment here that we will leak if there's no sc, and explain the cases when that can happen. @@ +175,5 @@ > RevokeURLRunnable(WorkerPrivate* aWorkerPrivate, > const nsAString& aURL) > + : WorkerMainThreadRunnable(aWorkerPrivate) > + , mURL(aURL) > + , mUnregisteredWithMainThreadGlobal(true) You know if this is a shared/service worker here too. @@ +203,5 @@ > + } else { > + // Walk up to top worker object. > + WorkerPrivate* wp = mWorkerPrivate; > + while (wp->GetParent()) { > + wp = wp->GetParent(); Same nit
Attachment #8599512 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 35•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=682bd72b34d1
Assignee | ||
Comment 36•9 years ago
|
||
Attachment #8598229 -
Attachment is obsolete: true
Assignee | ||
Comment 37•9 years ago
|
||
Attachment #8599512 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 38•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e84c560ac0b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/522f3549ec5b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e84c560ac0b https://hg.mozilla.org/mozilla-central/rev/522f3549ec5b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Depends on: CVE-2015-2743
Comment 40•9 years ago
|
||
Is there a reason the new member on nsIGlobalObject wasn't added next to the existing mIsDying member? Having the data members spread out like that is a bit weird.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 41•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #40) > Is there a reason the new member on nsIGlobalObject wasn't added next to the > existing mIsDying member? Having the data members spread out like that is a > bit weird. No reasons actually. Do you want me to move it?
Flags: needinfo?(amarchesini)
Comment 42•9 years ago
|
||
Please.
Assignee | ||
Comment 43•9 years ago
|
||
Attachment #8627246 -
Flags: review?(bzbarsky)
Comment 44•9 years ago
|
||
Comment on attachment 8627246 [details] [diff] [review] patch 3 r=me
Attachment #8627246 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 45•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd3716e4bd1
You need to log in
before you can comment on or make changes to this bug.
Description
•