Closed Bug 1477563 Opened 4 years ago Closed 4 years ago

Deep copy file descriptor sets when copying IPDL messages

Categories

(Core Graveyard :: Web Replay, defect)

defect
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Currently, copying an IPDL message in middleman processes only adds an extra reference on their file descriptor set.  This is fine if the new message will be forwarded to the recording process and the old one discarded (the usual case), but if the old one is received in the middleman process than any file descriptors it references will be closed and cannot then be sent to the recording process.  This problem showed up after a recent rebase, as one of the few messages handled in the middleman process (PContent::SetXPCOMProcessAttributes) now takes a file descriptor.

This patch fixes this by making a deep copy of file descriptor sets --- dup'ing all their descriptors --- when copying IPDL messages.
Attachment #8994029 - Flags: review?(nfroyd)
Comment on attachment 8994029 [details] [diff] [review]
patch

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

::: ipc/chromium/src/chrome/common/ipc_message.cc
@@ +148,5 @@
>  void Message::CopyFrom(const Message& other) {
>    Pickle::CopyFrom(other);
>  #if defined(OS_POSIX)
> +  if (other.file_descriptor_set_) {
> +    file_descriptor_set_ = new FileDescriptorSet;

Is it reasonable to assert nullness of this->file_descriptor_set_ here?  I know the code should Just Work otherwise, but it seems like we don't want to call CopyFrom on a Message that has a bunch of data associated with it?
Attachment #8994029 - Flags: review?(nfroyd) → review+
Sure, I'll add assertions that the message is initially empty.
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0cf4cad7ce
Deep copy file descriptor sets when copying IPDL messages, r=froydnj.
https://hg.mozilla.org/mozilla-central/rev/cb0cf4cad7ce
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8994029 [details] [diff] [review]
patch

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

::: ipc/chromium/src/chrome/common/file_descriptor_set_posix.cc
@@ +38,5 @@
> +void FileDescriptorSet::CopyFrom(const FileDescriptorSet& other)
> +{
> +  for (std::vector<base::FileDescriptor>::const_iterator
> +       i = other.descriptors_.begin(); i != other.descriptors_.end(); ++i) {
> +    int fd = IGNORE_EINTR(dup(i->fd));

For future reference: IGNORE_EINTR changes (-1, EINTR) to 0, so this would replace the fd with stdin if it were possible for dup to fail that way… but it can't.  (dup2 can, at least in theory, because the implicit close could cause blocking I/O; unlike close(), it's safely HANDLE_EINTR()able.)

But the reason I'm looking at this code is because I'm planning to refactor it, so there's probably no point in fixing that.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.