Merge branch 'close-range-2' into 'main'

ExecFork: Simplify closing and make it safe

See merge request apt-team/apt!525

3 files changed, 28 insertions, 66 deletions

diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 7f785f21c..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);
@@ -920,38 +921,27 @@ pid_t ExecFork(std::set<int> KeepFDs)
       signal(SIGWINCH,SIG_DFL);
       signal(SIGCONT,SIG_DFL);
       signal(SIGTSTP,SIG_DFL);
+      auto safeSetCloseExec = [](int fd, bool close)
+      {
+	 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);
+      };
 
-#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 +949,18 @@ pid_t ExecFork(std::set<int> 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/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")));
 }
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