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.

2 files changed, 19 insertions, 63 deletions

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<int> 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<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/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