Closed Bug 1234192 Opened 7 years ago Closed 7 years ago

OSFileSystem looks scary since it is threadsafe object but keeps a strong pointer to main-thread only mWindow

Categories

(Core :: DOM: Core & HTML, defect)

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: smaug, Assigned: baku)

Details

(Keywords: csectype-race, sec-high)

Attachments

(3 files)

http://mxr.mozilla.org/mozilla-central/source/dom/filesystem/OSFileSystem.h looks scary.
Class which has strong pointer to cycle collectable (and not ever traversed or unlinked - so leaking ?)
object inhering threadsafe refcounting from the base class.
Does anything guarantee the object is always destroyed in the main thread?
Or perhaps OSFileSystem is always used just in the main thread even though there is threadsafe refcounting?
But don't we anyhow end up leaking?
Flags: needinfo?(amarchesini)
Attached patch fsb.patchSplinter Review
I didn't use nsProxyRelease because it doesn't work properly with NS_INLINE_DECL_REFCOUNTING classes.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8702562 - Flags: review?(bugs)
But why does the patch fix the possible leak? We still keep strong ref to a Window and not traverse/unlink it.
Comment on attachment 8702562 [details] [diff] [review]
fsb.patch

This should be fine for the security issue, but I think we should fix the leak issue in this bug too.
Attachment #8702562 - Flags: review?(bugs) → review+
Attached patch fsb2.patchSplinter Review
Attachment #8702676 - Flags: review?(bugs)
Attachment #8702676 - Flags: review?(bugs) → review+
I landed these 2 patches because the OSFileSystem obj is used only in nightly and protected behind pref.
Not just debug, a consistent crash in b2g opt, and not just b2g, an intermittent crash or assertion on desktop.
Even though this is disabled in Nightly, I'm going to mark it sec-high for completeness.
Group: dom-core-security → core-security-release
This is my bad. Sorry!
We fixed the security issue, but the traversing/unlinking is now wrong.
mFileSystem is owned by several different Directories, so we can just go from
Directory->fs->mWindow.

(This is all still not enabled-by-default-expect-in-Nightly code)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I hadn't realized the same FileSystem object is being reused, but DataTransfer certainly does that.
> (This is all still not enabled-by-default-expect-in-Nightly code)

I'm removing this 'reuse' thing in another patch that you are reviewing.
But if you prefer I submit a patch for that here.
Attached patch reuse.patchSplinter Review
Attachment #8707395 - Flags: review?(bugs)
Comment on attachment 8707395 [details] [diff] [review]
reuse.patch

Thanks.

This is still a bit fragile. We should somehow be able to guarantee fs isn't reused.

Would it be possible have an Init method on Directory where you pass window and dirname and it creates OSFileSystem which the Directory uses.
(I'm trying to recall why FileSystem needs a pointer to the global)
Attachment #8707395 - Flags: review?(bugs) → review+
> Would it be possible have an Init method on Directory where you pass window
> and dirname and it creates OSFileSystem which the Directory uses.
> (I'm trying to recall why FileSystem needs a pointer to the global)

The global is used to create objects by the tasks. Currently we have a window, but as you know, in other patches I'm moving it to a nsISupports.
I'll make your proposal in one of the other patches you are reviewing.
https://hg.mozilla.org/mozilla-central/rev/061115751346
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.