From 1f1819396f0171ec1b669f97bd674329133f5bef Mon Sep 17 00:00:00 2001 From: Olly Betts Date: Fri, 2 Dec 2016 15:48:31 +1300 Subject: [PATCH] Avoid opendir()/readdir() in closefrom() implementation These functions can call malloc(), which isn't safe to do between fork() and exec() in a multi-threaded program, but after fork() is exactly where you want to use closefrom(). Instead we now use getdirentries(), which isn't as portable, but is available on both the platforms we need it on - Linux and OS X. (cherry picked from commit 9ee6d2412eeca6c24cb4da33d80dbcf9986347dc) --- xapian-core/common/closefrom.cc | 90 ++++++++++++++++++++++------------------- xapian-core/configure.ac | 6 +-- xapian-core/tests/unittest.cc | 29 +++++++++++++ 3 files changed, 80 insertions(+), 45 deletions(-) diff --git a/xapian-core/common/closefrom.cc b/xapian-core/common/closefrom.cc index b5ae41f76..d608dc905 100644 --- a/xapian-core/common/closefrom.cc +++ b/xapian-core/common/closefrom.cc @@ -1,7 +1,7 @@ /** @file closefrom.cc * @brief Implementation of closefrom() function. */ -/* Copyright (C) 2010,2011,2012 Olly Betts +/* Copyright (C) 2010,2011,2012,2016 Olly Betts * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -66,58 +66,63 @@ Xapian::Internal::closefrom(int fd) if (fcntl(fd, F_CLOSEM, 0) >= 0) return; #elif defined __linux__ || defined __APPLE__ - // The loop might close the fd associated with dir if we don't take - // special care to avoid that by either skipping this fd in the closing - // loop (if dirfd() is available) or making sure we have a free fd below - // the first we close in the loop. -#if !defined HAVE_DIRFD && !defined dirfd - // Make sure that the lowest fd we have been asked to close is closed, and - // then raise this lower bound - this should ensure that opendir() gets - // an fd below the new lower bound. - while (close(fd) < 0 && errno == EINTR) { } - ++fd; -#endif #if 0 // Some platforms (e.g. AIX) have /proc//fd but not /proc/self - if - // any such platforms don't have either closefrom() or F_CLOSEM then this - // code can be used. - string path = "/proc/"; - path += str(getpid()); - path += "/fd"; - DIR * dir = opendir(path.c_str()); + // any such platforms don't have either closefrom() or F_CLOSEM but do + // have getdirentries() then this code can be used. + char path[6 + sizeof(pid_t) * 3 + 4]; + sprintf(path, "/proc/%ld/fd", long(getpid())); #elif defined __linux__ - DIR * dir = opendir("/proc/self/fd"); + const char * path = "/proc/self/fd"; #elif defined __APPLE__ // Mac OS X - DIR * dir = opendir("/dev/fd"); + const char * path = "/dev/fd"; #endif - if (dir) { + int dir = open(path, O_RDONLY|O_DIRECTORY); + if (dir >= 0) { + off_t base = 0; while (true) { + char buf[1024]; errno = 0; - struct dirent *entry = readdir(dir); - if (entry == NULL) { - closedir(dir); - // Fallback if readdir() or closedir() fails. - if (errno) break; + // We use getdirentries() instead of opendir()/readdir() here + // because the latter can call malloc(), which isn't safe to do + // between fork() and exec() in a multi-threaded program. + ssize_t c = getdirentries(dir, buf, sizeof(buf), &base); + if (c == 0) { + close(dir); return; } - char ch; - ch = entry->d_name[0]; - if (ch < '0' || ch > '9') - continue; - int n = atoi(entry->d_name); - if (n >= fd) { -#if defined HAVE_DIRFD || defined dirfd - if (n == dirfd(dir)) continue; -#endif + if (c < 0) { + // Fallback if getdirentries() fails. + break; + } + struct dirent *d; + for (ssize_t pos = 0; pos < c; pos += d->d_reclen) { + d = reinterpret_cast(buf + pos); + const char * leaf = d->d_name; + if (leaf[0] < '0' || leaf[0] > '9') { + // Skip '.' and '..'. + continue; + } + int n = atoi(leaf); + if (n < fd) { + // FD below threshold. + continue; + } + if (n == dir) { + // Don't close the fd open on the directory. + continue; + } #ifdef __linux__ // Running under valgrind causes some entries above the - // reported RLIMIT_NOFILE value to appear in /proc/self/fd - // (https://bugs.kde.org/show_bug.cgi?id=191758). If we try - // to close these, valgrind issues a warning about trying to - // close an invalid file descriptor. These entries start at - // 1024, so we check that value first so we can usually avoid - // having to read the fd limit when we're not running under - // valgrind. + // reported RLIMIT_NOFILE value to appear in + // /proc/self/fd - see: + // https://bugs.kde.org/show_bug.cgi?id=191758 + // + // If we try to close these, valgrind issues a warning about + // trying to close an invalid file descriptor. These entries + // start at 1024, so we check that value first so we can + // usually avoid having to read the fd limit when we're not + // running under valgrind. if (n >= 1024) { if (maxfd < 0) maxfd = get_maxfd(); @@ -129,6 +134,7 @@ Xapian::Internal::closefrom(int fd) while (close(n) < 0 && errno == EINTR) { } } } + close(dir); } #endif if (maxfd < 0) diff --git a/xapian-core/configure.ac b/xapian-core/configure.ac index fdb683a66..6c93967b8 100644 --- a/xapian-core/configure.ac +++ b/xapian-core/configure.ac @@ -439,9 +439,9 @@ AC_CHECK_FUNCS([setenv _putenv_s]) dnl Check for a more efficient way of closing fds during daemonisation. dnl Apparently closefrom() is available on at least "Solaris 9 or later, NetBSD dnl 3.0 or later, OpenBSD 3.5 or later". If we don't have closefrom(), then -dnl dirfd() and getrlimit are useful for an efficient implementation on some -dnl platforms. -AC_CHECK_FUNCS([closefrom dirfd getrlimit]) +dnl getdirentries() and getrlimit() are useful for an efficient implementation +dnl on some platforms. +AC_CHECK_FUNCS([closefrom getdirentries getrlimit]) dnl See if ftime returns void (as it does on mingw) AC_MSG_CHECKING([return type of ftime]) diff --git a/xapian-core/tests/unittest.cc b/xapian-core/tests/unittest.cc index 991e7bb04..ee074ddf8 100644 --- a/xapian-core/tests/unittest.cc +++ b/xapian-core/tests/unittest.cc @@ -23,10 +23,13 @@ #include #include +#include #include #include #include +#include "safeunistd.h" + #define XAPIAN_UNITTEST static const char * unittest_assertion_failed = NULL; #define UNITTEST_CHECK_EXCEPTION \ @@ -67,6 +70,7 @@ using namespace std; } while (0) // Code we're unit testing: +#include "../common/closefrom.cc" #include "../common/errno_to_string.cc" #include "../common/fileutils.cc" #include "../common/serialise-double.cc" @@ -547,6 +551,30 @@ static bool test_strbool1() return true; } +static bool test_closefrom1() +{ +#ifndef __WIN32__ + // Simple test. + closefrom(8); + + // Simple test when there are definitely no fds to close. + closefrom(42); + + // Test passing a really high threshold. + closefrom(INT_MAX); + + // Open some fds and check the expected ones are closed. + TEST_EQUAL(dup2(1, 14), 14); + TEST_EQUAL(dup2(1, 15), 15); + TEST_EQUAL(dup2(1, 18), 18); + closefrom(15); + TEST_EQUAL(close(14), 0); + TEST(close(15) == -1 && errno == EBADF); + TEST(close(18) == -1 && errno == EBADF); +#endif + return true; +} + static const test_desc tests[] = { TESTCASE(simple_exceptions_work1), TESTCASE(class_exceptions_work1), @@ -561,6 +589,7 @@ static const test_desc tests[] = { TESTCASE(sortableserialise1), TESTCASE(tostring1), TESTCASE(strbool1), + TESTCASE(closefrom1), END_OF_TESTCASES }; -- 2.11.4.GIT