Closed Bug 1456911 Opened 7 years ago Closed 6 years ago

LaunchApp doesn't clear close-on-exec when a file descriptor is mapped to itself

Categories

(Core :: IPC, defect, P2)

Product:
Core
Component:
IPC
Platform:
Unspecified
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: jld, Assigned: jld)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1456911 - Prelude: Fix unified build breakage exposed by the next patch.
(deleted), text/x-review-board-request
froydnj
: review+
Details
Bug 1456911 - Rewrite the fd shuffling to be simpler & handle identity mappings correctly.
(deleted), text/x-review-board-request
froydnj
: review+
Details
I haven't tested this yet, but it seems to be the case from code inspection: if a file descriptor in LaunchOptions::fds_to_remap has the same numeric value in the parent as the child, it won't have its close-on-exec bit cleared. (In contrast, the general case dup2()s the descriptor, which does clear close-on-exec.) So if the fd was close-on-exec, which is the general best practice, it will be closed on exec instead of inherited. Note that dup2(x, x) is defined as a no-op and POSIX specifically says it won't change the close-on-exec bit, so that will need to be done explicitly. This *probably* isn't a problem in practice, because the low-valued fds we map things to will *probably* wind up occupied by long-lived fds in the parent process… but maybe not. In any case I'd like to rule this out as a possible cause of bug 1455828. Interestingly, Chromium's current version of this code doesn't seem to handle that either, so maybe I'm missing something... or maybe they have a 10-year-old bug.
Another possibility is to just rip out most of this code, and instead dup() as needed to keep the source and destination fd ranges disjoint, at which point everything gets dup2()ed and this isn't a special case. That could also help for the posix_spawn implementation on Mac, which isn't using this and should be (that's another bug I'm in the middle of filing), because I've heard that OS X doesn't like identity mappings with posix_spawn_file_actions_adddup2 (which, if true, is an OS bug, and also I have no idea how it interacts with POSIX_SPAWN_CLOEXEC_DEFAULT).
I've tested this, and an identity-mapped close-on-exec file does in fact fail to be inherited. As for comment #1: From looking at the kernel code, OS X's implementation of posix_spawn_file_actions_adddup2 seems to correct for the old==new case, and it ought to exempt the fd from POSIX_SPAWN_CLOEXEC_DEFAULT, but I haven't tested that yet. But, the current file_descriptor_shuffle assumes it can dup() in the child process, which will be a problem for bug 1456919, so that's another reason to rewrite.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8983991 [details] Bug 1456911 - Prelude: Fix unified build breakage exposed by the next patch. https://reviewboard.mozilla.org/r/249864/#review256226
Attachment #8983991 - Flags: review?(nfroyd) → review+
Comment on attachment 8983992 [details] Bug 1456911 - Rewrite the fd shuffling to be simpler & handle identity mappings correctly. https://reviewboard.mozilla.org/r/249866/#review256270 This is nice. ::: ipc/chromium/src/base/process_util.h:92 (Diff revision 1) > -// Close all file descriptors, expect those which are a destination in the > -// given multimap. Only call this function in a child process where you know > +// Close all file descriptors, except those for which the given > +// function returns true (and std{in,out,err}). Only call this Maybe rewrite this as "Close all file descriptors, except for std{in,out,err} and those for which the given function returns true."? I took the current wording to mean the function could return std{in,out,err}, which is admittedly bizarre, and that might have just been me. ::: ipc/glue/FileDescriptorShuffle.h:47 (Diff revision 1) > + // Can fail (return false) on failure to duplicate fds. > + bool Init(MappingRef aMapping); > + > + // Accessor for the dup2() sequence. Do not use the returned value > + // or the fds contained in it after this object is destroyed. > + MappingRef Dup2Sequence() const { return mMapping; } Should this return value be `const`?
Attachment #8983992 - Flags: review?(nfroyd) → review+
Thanks for the review. (In reply to Nathan Froyd [:froydnj] from comment #6) > ::: ipc/chromium/src/base/process_util.h:92 > (Diff revision 1) > > -// Close all file descriptors, expect those which are a destination in the > > -// given multimap. Only call this function in a child process where you know > > +// Close all file descriptors, except those for which the given > > +// function returns true (and std{in,out,err}). Only call this > > Maybe rewrite this as "Close all file descriptors, except for > std{in,out,err} and those for which the given function returns true."? I > took the current wording to mean the function could return std{in,out,err}, > which is admittedly bizarre, and that might have just been me. Yeah, that looks a little odd; I'll reword. > ::: ipc/glue/FileDescriptorShuffle.h:47 > (Diff revision 1) > > + // Can fail (return false) on failure to duplicate fds. > > + bool Init(MappingRef aMapping); > > + > > + // Accessor for the dup2() sequence. Do not use the returned value > > + // or the fds contained in it after this object is destroyed. > > + MappingRef Dup2Sequence() const { return mMapping; } > > Should this return value be `const`? Making the Span itself const won't change anything, I don't think? And the pointer target type is already const.
Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/808a2fe2aaab Prelude: Fix unified build breakage exposed by the next patch. r=froydnj https://hg.mozilla.org/integration/autoland/rev/69ffa8eab9d9 Rewrite the fd shuffling to be simpler & handle identity mappings correctly. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/808a2fe2aaab https://hg.mozilla.org/mozilla-central/rev/69ffa8eab9d9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: