Open fstream files in O_CLOEXEC mode when possible.

[mclow.lists]

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

[danalbert]

In D59839#1443731, @mclow.lists wrote:

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.

[mclow.lists]

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

[ldionne]

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.

[danalbert]

In D59839#1444061, @mclow.lists wrote:

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.

In D59839#1444554, @ldionne wrote:

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.

[danalbert]

Ping?

[ldionne]

libcxx/include/__config

1460 ↗

(On Diff #192335)

Please leave a comment explaining what this is.

[danalbert]

Add a comment explaining _LIBCPP_FOPEN_CLOEXEC_MODE.

[ldionne]

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.

[danalbert]

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]

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]

libcxx/include/__config

1464 ↗

(On Diff #193168)

That was my impression, yes.

[ldionne]

libcxx/include/__config

1464 ↗

(On Diff #193168)

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

[danalbert]

Only opt-in for bionic

[danalbert]

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]

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]

@EricWF Any concerns, or can I merge this?

[danalbert]

@EricWF ping

libcxx/trunk/include/__config

 #    else
 #      pragma comment(lib, "libc++.lib")
 #    endif
 #  endif // defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
 #endif // _LIBCPP_NO_AUTO_LINK
 
 #define _LIBCPP_UNUSED_VAR(x) ((void)(x))
 
+// Configures the fopen close-on-exec mode character, if any. This string will
+// be appended to any mode string used by fstream for fopen/fdopen.
+//
+// Not all platforms support this, but it helps avoid fd-leaks on platforms that
+// do.
+#if defined(__BIONIC__)
+#  define _LIBCPP_FOPEN_CLOEXEC_MODE "e"
+#else
+#  define _LIBCPP_FOPEN_CLOEXEC_MODE
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

libcxx/trunk/include/fstream

 }
 
 template <class _CharT, class _Traits>
 const char* basic_filebuf<_CharT, _Traits>::__make_mdstring(
     ios_base::openmode __mode) _NOEXCEPT {
   switch (__mode & ~ios_base::ate) {
   case ios_base::out:
   case ios_base::out | ios_base::trunc:
-    return "w";
+    return "w" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::out | ios_base::app:
   case ios_base::app:
-    return "a";
+    return "a" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in:
-    return "r";
+    return "r" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::out:
-    return "r+";
+    return "r+" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::out | ios_base::trunc:
-    return "w+";
+    return "w+" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::out | ios_base::app:
   case ios_base::in | ios_base::app:
-    return "a+";
+    return "a+" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::out | ios_base::binary:
   case ios_base::out | ios_base::trunc | ios_base::binary:
-    return "wb";
+    return "wb" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::out | ios_base::app | ios_base::binary:
   case ios_base::app | ios_base::binary:
-    return "ab";
+    return "ab" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::binary:
-    return "rb";
+    return "rb" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::out | ios_base::binary:
-    return "r+b";
+    return "r+b" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::out | ios_base::trunc | ios_base::binary:
-    return "w+b";
+    return "w+b" _LIBCPP_FOPEN_CLOEXEC_MODE;
   case ios_base::in | ios_base::out | ios_base::app | ios_base::binary:
   case ios_base::in | ios_base::app | ios_base::binary:
-    return "a+b";
+    return "a+b" _LIBCPP_FOPEN_CLOEXEC_MODE;
   default:
     return nullptr;
   }
   _LIBCPP_UNREACHABLE();
 }
 
 #ifndef _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>