Page MenuHomePhabricator

Open fstream files in O_CLOEXEC mode when possible.
ClosedPublic

Authored by danalbert on Mar 26 2019, 1:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

danalbert created this revision.Mar 26 2019, 1:09 PM

How did you decide which modes fell into "when possible"?

danalbert added a comment.EditedMar 26 2019, 2:55 PM

How did you decide which modes fell into "when possible"?

AIUI it's all the modes, the "whenever possible" applies to platform support. The configuration I have here is definitely non-exhaustive, but I also don't know how to come up with an exhaustive list. I was able to check Android and glibc easily enough since that's what in front of me, but I figure anyone else that wants this behavior can easily update __config.

I'm mostly worried about the change in behavior breaking existing programs.

I knew nothing about this issue so I read https://mail.gnome.org/archives/gtk-devel-list/2015-March/msg00038.html, and I'd like @danalbert to comment on it.

I'm mostly worried about the change in behavior breaking existing programs.

My initial hunch was that only programs that are accidentally leaking file descriptors would be affected by this since there isn't a way to get the FILE* or fd out of an fstream, but I suppose you could close one of the standard streams and then create an fstream to have it be in a known location (or dup/close then open an fstream and pass that fd number on to the exec'd process). I suspect these behaviors are not all that common, but I can undo the glibc behavior change here if that would be preferred. I suspect more users would want to avoid fd leaks than there are users depending on being able to pass an fstream's fd to an exec'd process, but I'm not the maintainer of that platform so I'm not really the one that should be making that decision.

I knew nothing about this issue so I read https://mail.gnome.org/archives/gtk-devel-list/2015-March/msg00038.html, and I'd like @danalbert to comment on it.

That article is pretty unconvincing IMO. I don't buy the argument that it's safer/easier to make sure you close all your opened file descriptors pre-exec than it is to ask the OS to track that for you whenever you open a file. The main reason the author gives ignores the existence of F_DUPFD_CLOEXEC.

Switching topics from whether this change is generally desirable, does anyone have any objections to the patch being submitted if I change __config so only Android gets this behavior by default? O_CLOEXEC is something we definitely prefer to have by default.

ldionne requested changes to this revision.Apr 1 2019, 11:25 AM
ldionne added inline comments.
libcxx/include/__config
1460 ↗(On Diff #192335)

Please leave a comment explaining what this is.

This revision now requires changes to proceed.Apr 1 2019, 11:25 AM
danalbert updated this revision to Diff 193168.Apr 1 2019, 1:27 PM

Add a comment explaining _LIBCPP_FOPEN_CLOEXEC_MODE.

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2019, 1:27 PM
ldionne accepted this revision.Apr 1 2019, 1:33 PM

I'm fine with this as a direction but it doesn't affect my platform. If it were my platform, I'd want to make REALLY sure that we're not going to break existing programs.

This revision is now accepted and ready to land.Apr 1 2019, 1:33 PM

I'm not really sure who speaks for glibc platforms. @EricWF? Like I said, if we're not comfortable making this change for glibc I don't have a problem backing that out, but I do think this is a more useful default behavior. It's fairly convoluted to pass a file descriptor from an fstream across an exec boundary intentionally so I somewhat doubt anyone is doing so intentionally. Anyone doing so unintentionally is just leaking an fd and any use of it on the other side of the exec is probably a bug.

smeenai added a subscriber: smeenai.Apr 1 2019, 1:46 PM
smeenai added inline comments.
libcxx/include/__config
1464 ↗(On Diff #193168)

To be clear, the plan is to limit this to Bionic and drop the glibc part, correct?

mclow.lists added inline comments.Apr 2 2019, 6:36 AM
libcxx/include/__config
1464 ↗(On Diff #193168)

That was my impression, yes.

ldionne added inline comments.Apr 2 2019, 8:32 AM
libcxx/include/__config
1464 ↗(On Diff #193168)

I'd be fine with that. Please update the review.

danalbert updated this revision to Diff 193345.Apr 2 2019, 12:42 PM

Only opt-in for bionic

danalbert marked 5 inline comments as done.Apr 2 2019, 12:43 PM
danalbert added inline comments.
libcxx/include/__config
1464 ↗(On Diff #193168)

Sorry, it didn't seem anyone had actually made that decision. I was still waiting for someone to say yes or no.

ldionne accepted this revision.Apr 2 2019, 12:45 PM

I'm fine with this as it only impacts Android. I'd let @EricWF give the final thumbs up since it'll probably fall on him if something goes wrong with Android :).

libcxx/include/__config
1464 ↗(On Diff #193168)

No worries -- I don't think it was clear either.

danalbert marked an inline comment as done.Apr 16 2019, 1:40 PM

@EricWF Any concerns, or can I merge this?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 12:30 PM