Closed Bug 965724 Opened 10 years ago Closed 10 years ago

Remote open() calls via IPDL for MediaCache

Categories

(Core :: Security, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: arroway, Assigned: jld)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

MediaCache and EncodedBufferCache open temporary cache files by calling NS_OpenAnonymousTemporaryFile(). When called from a content process, it should remote  open() and unlink() operations to a parent process via IPDL and pass back the file descriptor to the content process (as suggested in bug 906996 comment 9 and bug 940863).
This is the hasty implementation of bug 906996 comment #9 that I wrote while investigating bug 930258.  I'm not sure if it's doing things in the right places code-wise, but it seemed to work in my local testing.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=57db509ebc14
Attachment #8442564 - Flags: feedback?(roc)
Comment on attachment 8442564 [details] [diff] [review]
bug965724-remote-tempfiles-hg0.diff

Review of attachment 8442564 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I think it would be a bit nicer to have NS_OpenAnonymousTemporaryFile be the universal entry point and have it implement the IPC stuff internally if there is a ContentChild.
Attachment #8442564 - Flags: feedback?(roc) → feedback+
Now with the e10s-awareness moved into xpcom/io.  And I think at this point it makes sense for me to steal this bug; hopefully that's not a problem.

https://tbpl.mozilla.org/?tree=Try&rev=08bb04d973ce
Assignee: stephouillon → jld
Attachment #8442564 - Attachment is obsolete: true
Attachment #8444559 - Flags: review?(roc)
Comment on attachment 8444559 [details] [diff] [review]
bug965724-remote-tempfiles-hg1.diff

Review of attachment 8444559 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +3649,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return false;
> +    }
> +    *aFD = FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(prfd));
> +    PR_Close(prfd);

Doesn't this close *aFD as well?

I don't understand how we ensure the FD is kept open long enough to be received by the child but not leaked.
Attachment #8444559 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> > +    *aFD = FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(prfd));
> > +    PR_Close(prfd);
> 
> Doesn't this close *aFD as well?
> 
> I don't understand how we ensure the FD is kept open long enough to be
> received by the child but not leaked.

The ipc::FileDescriptor constructor, which is called implicitly on the first line, duplicates the file descriptor/handle.  So the PR_Close closes the one that was opened, but the FileDescriptor has its own copy (which it owns and closes in its dtor, after duplicating it again over IPC).  I can add a comment and/or an explicit constructor call to make all that clearer.

Also, because I'm feeling more paranoid now than I was before I asked for review: are there any existing tests that use MediaCache in a remote browser?  If not, is there one that could be adapted?
Flags: needinfo?(roc)
Comment on attachment 8444559 [details] [diff] [review]
bug965724-remote-tempfiles-hg1.diff

Review of attachment 8444559 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure B2G tests catch this.

::: dom/ipc/ContentParent.cpp
@@ +3649,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return false;
> +    }
> +    *aFD = FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(prfd));
> +    PR_Close(prfd);

Alright, please add a comment explaining how this works.
Attachment #8444559 - Flags: review- → review+
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/f9793892fc81
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1146416
You need to log in before you can comment on or make changes to this bug.