1477563 - Deep copy file descriptor sets when copying IPDL messages

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Nathan Froyd [:froydnj]]

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?

Comment 2

[Brian Hackett [Laid off!]]

Sure, I'll add assertions that the message is initially empty.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0cf4cad7ce
Deep copy file descriptor sets when copying IPDL messages, r=froydnj.

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cb0cf4cad7ce

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.