Closed Bug 1477563 Opened 4 years ago Closed 4 years ago

Deep copy file descriptor sets when copying IPDL messages


(Core Graveyard :: Web Replay, defect)

Not set


(firefox63 fixed)

Tracking Status
firefox63 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)




(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]

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

::: ipc/chromium/src/chrome/common/
@@ +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
Deep copy file descriptor sets when copying IPDL messages, r=froydnj.
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8994029 [details] [diff] [review]

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

::: ipc/chromium/src/chrome/common/
@@ +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.