Re: [PATCH] open: introduce O_NOSTD

Davide Libenzi
2009-08-27T22:55:57Z

On Thu, 27 Aug 2009, Eric Blake wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> According to Davide Libenzi on 8/25/2009 3:53 PM:
> >> Another solution is for the application to sanitize all newly-created
> >> fds: GNU coreutils provides a wrapper open_safer, which does nothing
> >> extra in the common case that open() returned 3 or larger, but calls
> >> fcntl(n,F_DUPFD,3)/close(n) before returning if n was less than 3.
> >> However, this leads to triple the syscall cost for every open() call
> >> if the process starts life with a std fd closed; and if O_CLOEXEC is
> >> not used, still leaves a window of time where the fd can be leaked
> >> through another thread's use of fork/exec.
> >
> > I think we can say that the vast majority of the software is not going to
> > notice the proposed open_safer(), performance-wise, since the first three
> > fds are always filled. So IMO the performance impact argument is a weak one.
> > If CLOEXEC semantics are needed in the open operation, F_DUPFD_CLOEXEC can
> > be used to match it.
>
> The current gnulib implementation of open_safer (pre-O_CLOEXEC support) is
> (roughly):
>
> /* Wrap open, to guarantee that a successful return value is >= 3. */
> int open_safer (const char *name, int flags, int mode)
> {
> int fd = open (name, flags, mode);
> if (0 <= fd && fd <= 2)
> {
> int dup = fcntl (fd, F_DUPFD, 3);
> int saved_errno = errno;
> close (fd);
> errno = saved_errno;
> fd = dup;
> }
> return fd;
> }
>
> which has the desired property of no overhead in the common case of all
> standard fds open. But it obviously mishandles the O_CLOEXEC flag.
> Here's a first cut at supporting it:
>
> int open_safer (const char *name, int flags, int mode)
> {
> int fd = open (name, flags, mode);
> if (0 <= fd && fd <= 2)
> {
> int dup = fcntl (fd, ((flags & O_CLOEXEC)
> ? F_DUPFD_CLOEXEC : F_DUPFD), 3);
> int saved_errno = errno;
> close (fd);
> errno = saved_errno;
> fd = dup;
> }
> return fd;
> }
>
> If the user requested open_safer(O_CLOEXEC), then we still have the
> desired property of no syscall overhead and no fd leak. But if the user
> intentionally does not pass O_CLOEXEC because they wanted to create an
> inheritable fd, but without stomping on standard fds, then this version
> still has a window for an fd leak. So let's try this version, which
> guarantees no fd leak, while still keeping the semantics of giving the
> user an inheritable fd outside the std fd range:
>
> int open_safer (const char *name, int flags, int mode)
> {
> int fd = open (name, flags | O_CLOEXEC, mode);
> if (0 <= fd && fd <= 2)
> {
> int dup = fcntl (fd, ((flags & O_CLOEXEC)
> ? F_DUPFD_CLOEXEC : F_DUPFD), 3);
> int saved_errno = errno;
> close (fd);
> errno = saved_errno;
> fd = dup;
> }
> else if (!(flags & O_CLOEXEC))
> {
> if ((flags = fcntl (fd, F_GETFD)) < 0
> || fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
> {
> int saved_errno = errno;
> close (fd);
> fd = -1;
> errno = saved_errno;
> }
> }
> return fd;
> }
>
> This solves the fd leak, and open_safer(O_CLOEXEC) is still cheap in the
> common case. But now the case of open_safer without O_CLOEXEC costs 3
> syscalls, regardless of whether the standard fds were already open (if we
> assumed there were no possibility of new FD_* flags, we could cut the
> common-case penalty from three to two by skipping the fcntl(fd,F_GETFD)
> and just using fcntl(fd,F_SETFD,0), but that's asking for problems).
>
> > While the patch is simple, IMO this is something that can be easily taken
> > care in glibc layers w/out huge drawbacks.
>
> I hope that my example shows why doing it in the kernel is desirable -
> there is no safe way to keep the pre-O_CLOEXEC efficiency using just the
> library, but there IS a way to do it with kernel support:
>
> int open_safer (const char *name, int flags, int mode)
> {
> return open (name, flags | O_NOSTD, mode);
> }

Can't the handling be done on close(), like (modulo some errno save/restore):

int safer_close(int fd) {
int error = close(fd);

if (fd < 3 && fd >= 0) {
if ((fd = open("/dev/null", O_RDWR)) > 2)
close(fd);
}

return error;
}

- Davide