From owner-freebsd-hackers@FreeBSD.ORG Tue Aug 9 22:41:14 2011 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2C26B1065675; Tue, 9 Aug 2011 22:41:14 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id C6F2D8FC08; Tue, 9 Aug 2011 22:41:13 +0000 (UTC) Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132]) by mx1.stack.nl (Postfix) with ESMTP id 8D33E359350; Wed, 10 Aug 2011 00:41:11 +0200 (CEST) Received: by turtle.stack.nl (Postfix, from userid 1677) id 79A92174D0; Wed, 10 Aug 2011 00:41:11 +0200 (CEST) Date: Wed, 10 Aug 2011 00:41:11 +0200 From: Jilles Tjoelker To: delphij@freebsd.org Message-ID: <20110809224111.GA15626@stack.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Cc: hackers@freebsd.org Subject: fdopendir() wrongly closes passed fd on error, union mess X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Aug 2011 22:41:14 -0000 While trying to use openat()/fdopendir()/fstatat() to improve performance of sh(1) pathname generation, I noticed that fdopendir() closes the passed file descriptor if it fails (such as when stable/8 silently ignores O_DIRECTORY even though it is defined in the header file). The below patch should fix this problem. I have changed the ugly DTF_REWIND code so it moves the reopened directory to the same file descriptor number, so as to minimize change there while ensuring the correct fd is closed. A later possibility is to fix the DTF_REWIND problem by reading union directories from a new descriptor obtained via fd2 = openat(fd, ".", O_RDONLY | O_DIRECTORY | O_CLOEXEC); and doing this unconditionally, retiring the DTF_REWIND flag. Reading the kernel code, it appears that calling getdirentries() on an open file description in a filesystem mounted with MNT_UNION (mount -o union) may change that open file description irreversibly to one pointing to the lower layer. This must not happen to the descriptor passed to fdopendir() or the descriptor returned via dirfd() since fchdir() and *at functions may not work properly with such a changed descriptor. Note: unionfs does not appear to mangle the open file description like MNT_UNION does, but it does need the duplicate removal code like MNT_UNION. Index: lib/libc/gen/opendir.c =================================================================== --- lib/libc/gen/opendir.c (revision 224739) +++ lib/libc/gen/opendir.c (working copy) @@ -75,6 +75,8 @@ __opendir2(const char *name, int flags) { int fd; struct stat statb; + DIR *dir; + int saved_errno; /* * stat() before _open() because opening of special files may be @@ -89,7 +91,13 @@ __opendir2(const char *name, int flags) if ((fd = _open(name, O_RDONLY | O_NONBLOCK | O_DIRECTORY)) == -1) return (NULL); - return __opendir_common(fd, name, flags); + dir = __opendir_common(fd, name, flags); + if (dir == NULL) { + saved_errno = errno; + _close(fd); + errno = saved_errno; + } + return (dir); } static int @@ -110,6 +118,7 @@ __opendir_common(int fd, const char *name, int fla int incr; int saved_errno; int unionstack; + int fd2; struct stat statb; dirp = NULL; @@ -199,14 +208,15 @@ __opendir_common(int fd, const char *name, int fla * which has also been read -- see fts.c. */ if (flags & DTF_REWIND) { - (void)_close(fd); - if ((fd = _open(name, O_RDONLY | O_DIRECTORY)) == -1) { + if ((fd2 = _open(name, O_RDONLY | O_DIRECTORY)) == -1) { saved_errno = errno; free(buf); free(dirp); errno = saved_errno; return (NULL); } + (void)_dup2(fd2, fd); + _close(fd2); } /* @@ -309,7 +319,6 @@ __opendir_common(int fd, const char *name, int fla fail: saved_errno = errno; free(dirp); - (void)_close(fd); errno = saved_errno; return (NULL); } -- Jilles Tjoelker