From 3dea3434bf2003b757d7ecdb5a8194c8a0c7023f Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 20 Oct 2025 20:02:17 +0200 Subject: ExecFork: Simplify closing and make it safe Implement a wrapper around FD_CLOEXEC and use it in place of the raw fcntl(). This wrapper _Exit()s the child if the operation failed. Restructure the close_range() handling to simply mark all fds for closing first and then "reopen" the ones we should keep. --- apt-pkg/contrib/fileutl.cc | 50 +++++++++++++++------------------------ test/integration/test-close-range | 32 ------------------------- 2 files changed, 19 insertions(+), 63 deletions(-) delete mode 100755 test/integration/test-close-range diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index 7f785f21c..fc4e047a5 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -920,38 +920,24 @@ pid_t ExecFork(std::set KeepFDs) signal(SIGWINCH,SIG_DFL); signal(SIGCONT,SIG_DFL); signal(SIGTSTP,SIG_DFL); + auto safeSetCloseExec = [](int fd, bool close) + { + if (fcntl(fd, F_SETFD, close ? FD_CLOEXEC : 0) == 0 || errno == EBADF) + return; + cerr << "FATAL -> Could not set close on exec " << strerror(errno) << endl; + _Exit(100); + }; -#ifdef CLOSE_RANGE_CLOEXEC - // Close all our file descriptors using close_range() if available. This is substantially - // faster than the naive method and may be faster than the /proc/self/fd one. +// If we don't have it, make the close_range() expression a failure; avoids #if inside the if/else +#ifndef CLOSE_RANGE_CLOEXEC +#define close_range(...) -1 +#endif + if (close_range(3, ~0U, CLOSE_RANGE_CLOEXEC) == 0) { - // Used for testing. - auto DebugCloseRange = _config->FindB("Debug::CloseRange", false); - int first = 3; for (auto keep : KeepFDs) - { - if (keep > first) - { - if (DebugCloseRange) - std::clog << "close_range(" << first << ", " << keep - 1 << ")" << std::endl; - if (close_range(first, keep - 1, CLOSE_RANGE_CLOEXEC)) - goto err; - } - first = keep + 1; - } - if (DebugCloseRange) - std::clog << "close_range(" << first << ", " << ~0U << ")" << std::endl; - if (close_range(first, ~0U, CLOSE_RANGE_CLOEXEC)) - goto err; - - return Process; - err: - if (DebugCloseRange) - std::clog << "close_range() failed" << std::endl; - // fallthrough + safeSetCloseExec(keep, false); } -#endif - if (DIR *dir = opendir("/proc/self/fd")) + else if (DIR *dir = opendir("/proc/self/fd")) { struct dirent *ent; while ((ent = readdir(dir))) @@ -959,16 +945,18 @@ pid_t ExecFork(std::set KeepFDs) int fd = atoi(ent->d_name); // If fd > 0, it was a fd number and not . or .. if (fd >= 3 && KeepFDs.find(fd) == KeepFDs.end()) - fcntl(fd,F_SETFD,FD_CLOEXEC); + safeSetCloseExec(fd, true); } closedir(dir); - } else { + } + else + { long ScOpenMax = sysconf(_SC_OPEN_MAX); // Close all of our FDs - just in case for (int K = 3; K != ScOpenMax; K++) { if(KeepFDs.find(K) == KeepFDs.end()) - fcntl(K,F_SETFD,FD_CLOEXEC); + safeSetCloseExec(K, true); } } } diff --git a/test/integration/test-close-range b/test/integration/test-close-range deleted file mode 100755 index 108fdea2d..000000000 --- a/test/integration/test-close-range +++ /dev/null @@ -1,32 +0,0 @@ -#!/bin/sh -set -e - -TESTDIR="$(readlink -f "$(dirname "$0")")" -. "$TESTDIR/framework" - -setupenvironment -configarchitecture 'amd64' - - -testsuccessequal "close_range(3, 4294967295)" apthelper drop-privs -o Debug::CloseRange=1 true - - -testsuccessequal "close_range(4, 4294967295)" apthelper drop-privs -o APT::Keep-Fds::=3 -o Debug::CloseRange=1 true - - -testsuccessequal "close_range(3, 4) -close_range(6, 4294967295)" apthelper drop-privs -o APT::Keep-Fds::=5 -o Debug::CloseRange=1 true - - -testsuccessequal "close_range(4, 4) -close_range(6, 4294967295)" apthelper drop-privs -o APT::Keep-Fds::=3 -o APT::Keep-Fds::=5 -o Debug::CloseRange=1 true - -testsuccessequal "close_range(4, 4) -close_range(7, 4294967295)" apthelper drop-privs -o APT::Keep-Fds::=3 -o APT::Keep-Fds::=5 -o APT::Keep-Fds::=6 -o Debug::CloseRange=1 true - -# Check with reverse order -testsuccessequal "close_range(4, 5) -close_range(7, 4294967295)" apthelper drop-privs -o APT::Keep-Fds::=6 -o APT::Keep-Fds::=3 -o Debug::CloseRange=1 true - -testsuccessequal "close_range(3, 4) -close_range(7, 4294967295)" apthelper drop-privs -o APT::Keep-Fds::=6 -o APT::Keep-Fds::=5 -o Debug::CloseRange=1 true -- cgit v1.2.3-70-g09d2 From bb1e6834e8a1cd6fd5b88b5a1962e9d813a080fc Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 20 Oct 2025 20:26:49 +0200 Subject: Correctly append or remove FD_CLOEXEC Rather than just setting the flags to 0 or FD_CLOEXEC, read the current value and append or remove as needed. --- apt-pkg/contrib/fileutl.cc | 8 ++++++-- apt-pkg/install-progress.cc | 6 ++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index fc4e047a5..69e6dc430 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -803,7 +803,8 @@ std::string flNormalize(std::string file) /*{{{*/ /* */ void SetCloseExec(int Fd,bool Close) { - if (fcntl(Fd,F_SETFD,(Close == false)?0:FD_CLOEXEC) != 0) + auto flags = fcntl(Fd, F_GETFD); + if (flags < 0 || fcntl(Fd, F_SETFD, Close ? flags | FD_CLOEXEC : flags & ~FD_CLOEXEC) != 0) { cerr << "FATAL -> Could not set close on exec " << strerror(errno) << endl; exit(100); @@ -922,7 +923,10 @@ pid_t ExecFork(std::set KeepFDs) signal(SIGTSTP,SIG_DFL); auto safeSetCloseExec = [](int fd, bool close) { - if (fcntl(fd, F_SETFD, close ? FD_CLOEXEC : 0) == 0 || errno == EBADF) + auto flags = fcntl(fd, F_GETFD); + if (flags < 0 && errno == EBADF) + return; + if (flags >= 0 && fcntl(fd, F_SETFD, close ? flags | FD_CLOEXEC : flags & ~FD_CLOEXEC) == 0) return; cerr << "FATAL -> Could not set close on exec " << strerror(errno) << endl; _Exit(100); diff --git a/apt-pkg/install-progress.cc b/apt-pkg/install-progress.cc index fe3324426..6ff8c3140 100644 --- a/apt-pkg/install-progress.cc +++ b/apt-pkg/install-progress.cc @@ -98,7 +98,8 @@ void PackageManagerProgressFd::StartDpkg() // FIXME: use SetCloseExec here once it taught about throwing // exceptions instead of doing _exit(100) on failure - fcntl(OutStatusFd,F_SETFD,FD_CLOEXEC); + if (auto flags = fcntl(OutStatusFd, F_GETFD); flags >= 0) + fcntl(OutStatusFd, F_SETFD, flags | FD_CLOEXEC); // send status information that we are about to fork dpkg WriteToStatusFd(GetProgressFdString("pmstatus", "dpkg-exec", StepsDone, StepsTotal, _("Running dpkg"))); @@ -180,7 +181,8 @@ void PackageManagerProgressDeb822Fd::StartDpkg() { // FIXME: use SetCloseExec here once it taught about throwing // exceptions instead of doing _exit(100) on failure - fcntl(OutStatusFd,F_SETFD,FD_CLOEXEC); + if (auto flags = fcntl(OutStatusFd, F_GETFD); flags >= 0) + fcntl(OutStatusFd, F_SETFD, flags | FD_CLOEXEC); WriteToStatusFd(GetProgressDeb822String("progress", nullptr, StepsDone, StepsTotal, _("Running dpkg"))); } -- cgit v1.2.3-70-g09d2