Date: Fri, 22 Jun 2012 09:42:29 GMT From: Jukka Ukkonen <jau@iki.fi> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/169320: Enhancement to allow fopen() to set O_CLOEXEC for open() Message-ID: <201206220942.q5M9gTCj020472@red.freebsd.org> Resent-Message-ID: <201206220950.q5M9o1IY046210@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 169320
>Category: kern
>Synopsis: Enhancement to allow fopen() to set O_CLOEXEC for open()
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: change-request
>Submitter-Id: current-users
>Arrival-Date: Fri Jun 22 09:50:01 UTC 2012
>Closed-Date:
>Last-Modified:
>Originator: Jukka Ukkonen
>Release: FreeBSD 9.0-STABLE
>Organization:
-----
>Environment:
FreeBSD sleipnir 9.0-STABLE FreeBSD 9.0-STABLE #0: Fri Jun 22 10:54:06 EEST 2012 root@sleipnir:/usr/obj/usr/src/sys/Sleipnir amd64
>Description:
This is a change request to allow a clean approach to fixing a short time window
between fopen() and fcntl() to set FD_CLOEXEC.
In threaded applications another thread could call exec() during that time
causing the newly created file descriptor to leak to the child program.
There is a lot of code with logic like...
fp = fopen (...);
..
fcntl(fileno(fp), F_SETFD, FD_CLOEXEC);
The currently existing alternative to plug this potential leak is change
the code to this method...
fd = open (..., O_CLOEXEC);
..
fp = fdopen (fd, ...);
Anyhow this leave the code still at least as hairy as the previous model using
a separate call to set FD_CLOEXEC.
A cleaner method would be extending the fopen() to allow a new mode character
as a companion to O_CLOEXEC flag for open().
FreeBSD already includes support for setting O_EXCL through the "x" mode
identifier. It would definitely not hurt adding a "c" mode character to
automatically set the file descriptor close-on-exec marker inside fopen().
With this approach the original code using fopen() & fcntl() could be
reduced to
fp = fopen (..., "...c");
to set the close-on-exec mark atomically inside open().
This would make the resulting code much cleaner wherever the close-on-exec
feature is needed.
The code would become also a bit less error prone because the flagging for
automatic close would be done in a centralized manner for all callers of
fopen().
While this change has been neither merged nor declined it does not make
sense to try to actively plug the little time windows for potential file
descriptor leaks to child programs.
==> Thus classification to serious/high.
>How-To-Repeat:
See the full description above.
>Fix:
Find attached a patch to implement the proposed change.
Patch attached with submission follows:
--- /usr/src/lib/libc/stdio/flags.c.orig 2012-06-22 09:25:12.000000000 +0300
+++ /usr/src/lib/libc/stdio/flags.c 2012-06-22 09:41:06.000000000 +0300
@@ -80,28 +80,43 @@
return (0);
}
- /* 'b' (binary) is ignored */
- if (*mode == 'b')
- mode++;
+ /*
+ * Because the order may not be universally deterministic,
+ * we are liberal and accept the add-on flags in any order.
+ */
- /* [rwa][b]\+ means read and write */
- if (*mode == '+') {
- mode++;
- ret = __SRW;
- m = O_RDWR;
- }
+ while (*mode) {
+ switch (*mode) {
+ case 'b':
+ /* 'b' (binary) is ignored */
+ break;
- /* 'b' (binary) can appear here, too -- and is ignored again */
- if (*mode == 'b')
- mode++;
+ case '+':
+ /* [rwa][b]\+ means read and write */
+ ret = __SRW;
+ m = O_RDWR;
+ break;
- /* 'x' means exclusive (fail if the file exists) */
- if (*mode == 'x') {
- if (m == O_RDONLY) {
- errno = EINVAL;
- return (0);
+ case 'x':
+ /* 'x' means exclusive (fail if the file exists) */
+ if (m == O_RDONLY) {
+ errno = EINVAL;
+ return (0);
+ }
+ o |= O_EXCL;
+ break;
+
+ case 'c':
+ /* Set the close-on-exec mark. */
+ o |= O_CLOEXEC;
+ break;
+
+ default:
+ /* We silently ignore what we do not know. */
+ break;
}
- o |= O_EXCL;
+
+ mode++;
}
*optr = m | o;
--- /usr/src/lib/libc/stdio/fopen.3.orig 2012-06-22 09:48:42.000000000 +0300
+++ /usr/src/lib/libc/stdio/fopen.3 2012-06-22 10:03:27.000000000 +0300
@@ -109,6 +109,22 @@
.St -isoC
and has no effect; the ``b'' is ignored.
.Pp
+If the
+.Fa mode
+string contains a flag
+.Dq Li c
+the file will be automatically closed when
+.Fn exec
+is called.
+This is a FreeBSD extension and companion to the O_CLOEXEC flag for
+.Fn open .
+This is a nifty tool for making code like system libraries more stable,
+but it also risks breaking the portability of code to other systems.
+There is no guarantee how other systems will handle an unspecified
+mode character.
+This implementation tries to be liberal and simply ignores all unknown
+mode characters.
+.Pp
Any created files will have mode
.Do Dv S_IRUSR
\&|
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201206220942.q5M9gTCj020472>