Handle the duplication of fd for thread link

ASSIGNED
Assigned to

Status

()

Core
IPC
ASSIGNED
3 years ago
3 years ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
In some case, we need to call dup() call to duplicate the fd from the message when we use thread link. I'm trying to handle this problem in ipc system.

e.g.
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/FenceUtilsGonk.cpp#89

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/FenceUtilsGonk.cpp#152
(Assignee)

Comment 1

3 years ago
Created attachment 8586057 [details] [diff] [review]
handle fd dup for thread link. v1

If we need to duplicate the fd in cross-thread case, we can just add the fd as:
aMsg->WriteFileDescriptor(FileDescriptor(fds[n], false, true));
And we can use the same code in message::Read() for both thread-link and process-link case.
Attachment #8586057 - Flags: review?(bent.mozilla)

Updated

3 years ago
Blocks: 1146214
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #0)
> In some case, we need to call dup() 

Shouldn't we need to remove all the related dup()s? Otherwise, applying attachment 8586057 [details] [diff] [review] might cause fd leak.
(Assignee)

Comment 3

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #0)
> > In some case, we need to call dup() 
> 
> Shouldn't we need to remove all the related dup()s? Otherwise, applying
> attachment 8586057 [details] [diff] [review] might cause fd leak.

The default behavior in attachment 8586057 [details] [diff] [review] doesn't do the dup(), so I think we can remove the related dup() call later. Please check the FileDescriptor ctor.
Status: NEW → ASSIGNED
My concern was about current general ipc implementation. I do not understand well around ipc, but it seems to assume that a fd is not duped when it is passed between threads.

one example is FileDescriptor::Assign(const FileDescriptor& aOther)
  https://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.h#119
(Assignee)

Comment 5

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> My concern was about current general ipc implementation. I do not understand
> well around ipc, but it seems to assume that a fd is not duped when it is
> passed between threads.
> 
> one example is FileDescriptor::Assign(const FileDescriptor& aOther)
>  
> https://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.h#119

This ipc::FileDescriptor is our ipc built-in type. It is still base on base::FileDescriptor[1]. This built-in type always duplicates fd before send[2]. So if you use ipc::FileDescriptor, it always duplicates. If you use your custom serializer, you can control that by the third parameter of base::FileDescriptor ctor.

[1]
https://hg.mozilla.org/mozilla-central/annotate/18a8ea7c2c62/ipc/glue/FileDescriptor.cpp#l106
[2]
https://hg.mozilla.org/mozilla-central/annotate/18a8ea7c2c62/ipc/glue/FileDescriptor.cpp#l104
Comment on attachment 8586057 [details] [diff] [review]
handle fd dup for thread link. v1

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

I'd like to see the next version with this stuff fixed, so r- for now.

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +32,5 @@
>        HANDLE_EINTR(close(descriptors_[i].fd));
>    }
>  }
>  
> +bool FileDescriptorSet::MaybeDupFileDescriptor() {

This should be void. You're not returning anything here currently, and you're not checking the return value anywhere else either.

@@ +33,5 @@
>    }
>  }
>  
> +bool FileDescriptorSet::MaybeDupFileDescriptor() {
> +  for (std::vector<base::FileDescriptor>::iterator

Nit: Please use |auto| here.

@@ +39,5 @@
> +    if (i->dup) {
> +      if (i->auto_close) {
> +        i->auto_close = false;
> +      } else {
> +        i->fd = dup(i->fd);

Please check dup() for failure (docs say it will return -1 on error). Something like:

  i->fd = dup(i->fd);
  MOZ_RELEASE_ASSERT(i->fd != -1);

I don't see an easy way to avoid crashing here if dup() fails. We could restructure a bunch of code to pass the result around but I doubt it's worth it yet.

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.h
@@ +31,5 @@
>    // Interfaces for building during message serialisation...
>  
> +  // Update the fd with the new duplicated fd number. Please check
> +  // base::FileDescriptor::dup flag.
> +  bool MaybeDupFileDescriptor();

void return.

::: ipc/chromium/src/chrome/common/ipc_message.cc
@@ +153,5 @@
>    return descriptor->fd >= 0;
>  }
>  
> +void Message::MaybeDupFileDescriptor() {
> +  if (file_descriptor_set()) {

This doesn't need to be tested, it will crash if allocation fails already.

::: ipc/glue/MessageLink.cpp
@@ +258,5 @@
>      mChan->AssertWorkerThread();
>      mChan->mMonitor->AssertCurrentThreadOwns();
>  
> +    if (mTargetChan) {
> +        msg->MaybeDupFileDescriptor();

Doesn't this need to be behind an #ifdef OS_POSIX?
Attachment #8586057 - Flags: review?(bent.mozilla) → review-
(Assignee)

Comment 7

3 years ago
Created attachment 8590158 [details] [diff] [review]
handle fd dup for thread link. v2
Attachment #8586057 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Comment on attachment 8586057 [details] [diff] [review]
handle fd dup for thread link. v1

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

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +32,5 @@
>        HANDLE_EINTR(close(descriptors_[i].fd));
>    }
>  }
>  
> +bool FileDescriptorSet::MaybeDupFileDescriptor() {

yes. it should be void. thx

::: ipc/glue/MessageLink.cpp
@@ +258,5 @@
>      mChan->AssertWorkerThread();
>      mChan->mMonitor->AssertCurrentThreadOwns();
>  
> +    if (mTargetChan) {
> +        msg->MaybeDupFileDescriptor();

thanks. we should use |OS_POSIX| since we only can pass fd over ipc in posix system.
(Assignee)

Comment 9

3 years ago
Created attachment 8590181 [details] [diff] [review]
handle fd dup for thread link. v3

use c++ auto keyword.
Attachment #8590158 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Comment on attachment 8590181 [details] [diff] [review]
handle fd dup for thread link. v3

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

::: ipc/chromium/src/base/file_descriptor_posix.h
@@ +23,2 @@
>  
> +  FileDescriptor(int ifd, bool iauto_close, bool idup = false)

We don't turn on dup flag by default, so we don't need to change current b2g code.

@@ +30,5 @@
>    // If true, this file descriptor should be closed after it has been used. For
>    // example an IPC system might interpret this flag as indicating that the
>    // file descriptor it has been given should be closed after use.
> +  // We will ignore auto_close flag if the fd is processed successfully in
> +  // thread link case.

Please check [1]. In thread link case, we still don't do the auto close even though we have this flag. I would like to add this comment, since I'm confused for this flag before.

[1]
https://hg.mozilla.org/mozilla-central/annotate/8f57f60ee58a/ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc#l17

@@ +38,5 @@
> +  // the original one. If auto_close is also true, auto_close will be ignored
> +  // and skip the dup() call. The fd is just transferred from send to receiver.
> +  // This flag is only used for thread link. In some case, we should use the
> +  // duplicated fd instead of sharing the same fd. Process link will ignore this
> +  // flag. It uses SCM_RIGHTS flag to handle this problem.

Ben, could you please check this comment? Is it clear?

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +32,5 @@
>        HANDLE_EINTR(close(descriptors_[i].fd));
>    }
>  }
>  
> +void FileDescriptorSet::MaybeDupFileDescriptor() {

void return type.

@@ +43,5 @@
> +        i->fd = dup(i->fd);
> +        // It's not a easy way to avoid crashing here if dup() fails.
> +        // We might need to restructure a bunch of code to pass the failed
> +        // around.
> +        MOZ_RELEASE_ASSERT(i->fd != -1);

Just assert here if we call dup() failed.

@@ +51,3 @@
>  }
>  
> +bool FileDescriptorSet::Add(const base::FileDescriptor& descriptor) {

merge AddAndAutoClose() and Add() to this one.

::: ipc/chromium/src/chrome/common/ipc_message.h
@@ +247,5 @@
>    //   iter: a Pickle iterator to the current location in the message.
>    bool ReadFileDescriptor(void** iter, base::FileDescriptor* descriptor) const;
> +  // Replace the fd with the new duplicated fd number(using dup()). Please check
> +  // base::FileDescriptor::dup comment. It's only used in thread link message
> +  // case.

Is this comment clear?

::: ipc/glue/MessageLink.cpp
@@ +258,5 @@
>      mChan->AssertWorkerThread();
>      mChan->mMonitor->AssertCurrentThreadOwns();
>  
> +    if (mTargetChan) {
> +#if defined(OS_POSIX)

only posix system has fd releated code.
(Assignee)

Updated

3 years ago
Attachment #8590181 - Flags: review?(bent.mozilla)
(Assignee)

Comment 11

3 years ago
Ben and Brian,
What's the different between [1] and [2]? Where do we use the sandbox?
I'm trying to add some new function in ipc fd passing. Should I consider the ipc code in sandbox folder?

[1]
security/sandbox/chromium/base/file_descriptor_posix.h
https://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/base/file_descriptor_posix.h#21

[2]
ipc/chromium/src/base/file_descriptor_posix.h
https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/file_descriptor_posix.h#18
Flags: needinfo?(netzen)
Flags: needinfo?(bent.mozilla)
[2] is an older fork from Chromium used in IPC code
[1] is a newer fork which diverged too much from [2] from Chromium and used in our Windows sandbox
It sounds like you want [1]
Flags: needinfo?(netzen)
ipc/chromium/src is used for IPC.  This bug appears to be about IPC messages, so I'd assume that that's the one this bug should be concerned with.

security/sandbox/chromium is used for sandboxing and not for IPC.  Local changes are made only if necessary, and must be listed in security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt
Comment on attachment 8590181 [details] [diff] [review]
handle fd dup for thread link. v3

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

Actually, can we just do everything in ThreadLink::SendMessage() and not modify any chromium code at all? I don't think this is a useful modification to the chromium code, and we apparently only need to do dup anything in this one ThreadLink case...

How does that sound?
Attachment #8590181 - Flags: review?(bent.mozilla) → review-
(Assignee)

Comment 15

3 years ago
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #14)
> Comment on attachment 8590181 [details] [diff] [review]
> handle fd dup for thread link. v3
> 
> Review of attachment 8590181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, can we just do everything in ThreadLink::SendMessage() and not
> modify any chromium code at all? I don't think this is a useful modification
> to the chromium code, and we apparently only need to do dup anything in this
> one ThreadLink case...
> 
> How does that sound?

Yes, we only do the dup at thread link case. I'm also like to do the dup in ThreadLink::SendMessage(), but we can't access the protect or private fd member through Message and FDSet type. I should export them or have friend declaration. That's why I create the MaybeDupFileDescriptor() function in Message and FDSet class.
Ben, what do you think?

Updated

3 years ago
No longer blocks: 1146214
Comment on attachment 8590181 [details] [diff] [review]
handle fd dup for thread link. v3

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

Bah, you're right, that's a mess. I read too fast again.

Also, presumably you need to change other places in the code that used to manually dup() these fds, right? Where is that patch?

How about we try this:

::: ipc/chromium/src/base/file_descriptor_posix.h
@@ +14,5 @@
>  // WARNING: (Chromium only) There are subtleties to consider if serialising
>  // these objects over IPC. See comments in chrome/common/ipc_message_utils.h
>  // above the template specialisation for this structure.
>  // -----------------------------------------------------------------------------
>  struct FileDescriptor {

Hopefully you can just revert all the changes in this file. We don't need a 'dup' flag on the object, and then the rest of the changes here don't need to be made.

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +35,5 @@
>  
> +void FileDescriptorSet::MaybeDupFileDescriptor() {
> +  for (auto i = descriptors_.begin(); i != descriptors_.end(); ++i) {
> +    // Please check base::FileDescriptor::dup comment.
> +    if (i->dup) {

Just always do this, no flag needed.

@@ +51,3 @@
>  }
>  
> +bool FileDescriptorSet::Add(const base::FileDescriptor& descriptor) {

Revert.

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.h
@@ +30,5 @@
>    // ---------------------------------------------------------------------------
>    // Interfaces for building during message serialisation...
>  
> +  // Update the fd with the new duplicated fd number.
> +  void MaybeDupFileDescriptor();

s/MaybeDupFileDescriptor/DupFileDescriptors/

@@ +36,2 @@
>    // Add a descriptor to the end of the set. Returns false iff the set is full.
> +  bool Add(const base::FileDescriptor& descriptor);

I don't think this is needed.

::: ipc/chromium/src/chrome/common/ipc_message.cc
@@ +152,5 @@
>  
>    return descriptor->fd >= 0;
>  }
>  
> +void Message::MaybeDupFileDescriptor() {

s/MaybeDupFileDescriptor/DupFileDescriptors/

::: ipc/chromium/src/chrome/common/ipc_message.h
@@ +248,5 @@
>    bool ReadFileDescriptor(void** iter, base::FileDescriptor* descriptor) const;
> +  // Replace the fd with the new duplicated fd number(using dup()). Please check
> +  // base::FileDescriptor::dup comment. It's only used in thread link message
> +  // case.
> +  void MaybeDupFileDescriptor();

s/MaybeDupFileDescriptor/DupFileDescriptors/
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 17

3 years ago
We need both case for thread link, so I still need to have more parameter in structure FileDescriptor.

[1] Don't need dup() for thread link case:
https://hg.mozilla.org/mozilla-central/annotate/de27ac2ab94f/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#l106
[2] Need dup() for thread link case:
https://hg.mozilla.org/mozilla-central/annotate/388f5861dc7d/gfx/layers/ipc/FenceUtilsGonk.cpp#l158

For [2], we have another serializer for process link case at [3].
[3] Don't need dup for process link case:
https://hg.mozilla.org/mozilla-central/annotate/388f5861dc7d/gfx/layers/ipc/FenceUtilsGonk.cpp#l59

[2] is almost similar to [3]. The difference is just the dup() call.
If we have one more additional parameter in FileDescriptor, we can merge these two Fence structure into one.

The serializer code in [2] and [3] can be merged into:
aMsg->WriteFileDescriptor(FileDescriptor(originalFd, false, true));
We don't need to care about using thread link or process link, just pass the third parameter to ipc system for dup() if needs.

We don't need dup in [1]. The code will like:
aMsg->WriteFileDescriptor(FileDescriptor(originalFd, false));
IPC doesn't handle dup() by default.

Are these use cases clear?
Flags: needinfo?(bent.mozilla)
Hrm, I don't think it makes sense for us to sometimes dup and sometimes not in the thread link case... To me it makes more sense to always do the same thing (dup) and fix all the callers/receivers to deal with that. What do you think?
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 19

3 years ago
Created attachment 8596379 [details] [diff] [review]
Part1: always dup fd in ipc system for thread link. v1

always dup the fd in thread link case.
update some comments for auto_close flag and fd duplication.
Attachment #8590181 - Attachment is obsolete: true
Attachment #8596379 - Flags: review?(bent.mozilla)
(Assignee)

Comment 20

3 years ago
Created attachment 8596384 [details] [diff] [review]
Part2: use same flow in both thread link and process link case for MagicGrallocBufferHandle.

In part1 patch, ipc system will help to duplicate the fd in thread link case. We should close the fd in receiver side in thread link case, since we get the GB from SharedBufferManager directly.
This patch try to use the same GraphicBuffer init flow for both thread link and process link case.
The overheads of this patch are the new operation and the GB unflatten call.
Attachment #8596384 - Flags: review?(sotaro.ikeda.g)
Attachment #8596384 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 21

3 years ago
Created attachment 8596385 [details] [diff] [review]
Part3: remove useless fd comment in FenceHandle.

remove the use less fd dup comment, since we don't call the dup() anymore.
Attachment #8596385 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 22

3 years ago
Comment on attachment 8596379 [details] [diff] [review]
Part1: always dup fd in ipc system for thread link. v1

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

We need to modify the MagicGrallocBufferHandle impl[1] since we have additional dup() call. That will be in another patch.

Ben, could you please check ipc::FileDescriptor[2]? I only see the ipc fd passing call here for windows platform. 
In [3], we pass the duplicated fd with auto_close flag. The receiver will just receive this duplicated fd in thread link case.
If we need to pass fd in windows platform, we should use the ipc::FileDescriptor type. The behave is still as [3].

[1]
https://hg.mozilla.org/mozilla-central/annotate/a5af73b32ac8/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#l106
[2]
https://dxr.mozilla.org/mozilla-central/source/ipc/glue/FileDescriptor.cpp
[3]
https://hg.mozilla.org/mozilla-central/annotate/a5af73b32ac8/ipc/glue/FileDescriptor.cpp#l107

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +46,5 @@
> +      i->fd = dup(i->fd);
> +      if (i->fd != -1) {
> +        NS_WARNING("Failed to duplicate file handle.");
> +      }
> +    }

If we have auto_close flag, we just pass that fd to receiver without dup().

::: ipc/chromium/src/chrome/common/ipc_message.h
@@ +245,5 @@
> +  // The receiver will get a duplicated fd in thread-link case, so the sender or
> +  // receiver can skip the manual dup() function.
> +  // The fd should still be valid at the time of transmission. If sender can't
> +  // make sure the fd lifetime, sender should consider to duplicate the fd and
> +  // setup the auto_close flag.

add comment that we do an additional dup() for thread link.
Comment on attachment 8596379 [details] [diff] [review]
Part1: always dup fd in ipc system for thread link. v1

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

So please make sure that this code is tested, both on POSIX and on Windows. We need to be 100% sure that we are not leaking fds in the process-link and the thread-link cases.

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +43,5 @@
> +    // fd even if it has auto_close flag in thread link case.
> +    // Please check FileDescriptorSet::~FileDescriptorSet().
> +    if (!i->auto_close) {
> +      i->fd = dup(i->fd);
> +      if (i->fd != -1) {

This test is reversed, I don't understand how anything works like this! Has this code been tested?

@@ +44,5 @@
> +    // Please check FileDescriptorSet::~FileDescriptorSet().
> +    if (!i->auto_close) {
> +      i->fd = dup(i->fd);
> +      if (i->fd != -1) {
> +        NS_WARNING("Failed to duplicate file handle.");

It think this should be MOZ_CRASH. Otherwise we're going to probably use a bad fd somewhere later and not know why.

::: ipc/chromium/src/chrome/common/ipc_message.cc
@@ +157,5 @@
>    return descriptor->fd >= 0;
>  }
>  
> +void Message::DupFileDescriptors() {
> +  file_descriptor_set()->DupFileDescriptors();

Don't call this, it will allocate a new fdset if one didn't exist.

Instead do something like:

  if (auto* fdset = file_descriptor_set_.get()) {
    fdset ->DupFileDescriptors();
  }
Attachment #8596379 - Flags: review?(bent.mozilla) → review-
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #23)
> This test is reversed, I don't understand how anything works like this! Has
> this code been tested?

Ah, I see, we were just warning but not doing anything. MOZ_CRASH will be useful here.
(Assignee)

Updated

3 years ago
Attachment #8596384 - Flags: review?(sotaro.ikeda.g)
Attachment #8596384 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8596385 - Flags: review?(sotaro.ikeda.g)
You need to log in before you can comment on or make changes to this bug.