From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 27 Oct 2023 21:27:20 +0000 (+0000)
Subject: Exit without asserting when helper process startup fails (#1543)
X-Git-Tag: SQUID_6_5~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6014c6648a2a54a4ecb7f952ea1163e0798f9264;p=thirdparty%2Fsquid.git

Exit without asserting when helper process startup fails (#1543)

... to dup() after fork() and before execvp().

Assertions are for handling program logic errors. Helper initialization
code already handled system call errors correctly (i.e. by exiting the
newly created helper process with an error), except for a couple of
assert()s that could be triggered by dup(2) failures.

This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/ipc-assert.html
where it was filed as 'Assertion in Squid "Helper" Process Creator'.
---

diff --git a/src/ipc.cc b/src/ipc.cc
index 40d34b4755..1afc4d5cf3 100644
--- a/src/ipc.cc
+++ b/src/ipc.cc
@@ -22,6 +22,11 @@
 
 #include <chrono>
 #include <thread>
+#include <cstdlib>
+
+#if HAVE_UNISTD_H
+#include <unistd.h>
+#endif
 
 static const char *hello_string = "hi there\n";
 #ifndef HELLO_BUF_SZ
@@ -362,6 +367,22 @@ ipcCreate(int type, const char *prog, const char *const args[], const char *name
     }
 
     PutEnvironment();
+
+    // A dup(2) wrapper that reports and exits the process on errors. The
+    // exiting logic is only suitable for this child process context.
+    const auto dupOrExit = [prog,name](const int oldFd) {
+        const auto newFd = dup(oldFd);
+        if (newFd < 0) {
+            const auto savedErrno = errno;
+            debugs(54, DBG_CRITICAL, "ERROR: Helper process initialization failure: " << name <<
+                   Debug::Extra << "helper (CHILD) PID: " << getpid() <<
+                   Debug::Extra << "helper program name: " << prog <<
+                   Debug::Extra << "dup(2) system call error for FD " << oldFd << ": " << xstrerr(savedErrno));
+            _exit(EXIT_FAILURE);
+        }
+        return newFd;
+    };
+
     /*
      * This double-dup stuff avoids problems when one of
      *  crfd, cwfd, or debug_log are in the rage 0-2.
@@ -369,17 +390,16 @@ ipcCreate(int type, const char *prog, const char *const args[], const char *name
 
     do {
         /* First make sure 0-2 is occupied by something. Gets cleaned up later */
-        x = dup(crfd);
-        assert(x > -1);
-    } while (x < 3 && x > -1);
+        x = dupOrExit(crfd);
+    } while (x < 3);
 
     close(x);
 
-    t1 = dup(crfd);
+    t1 = dupOrExit(crfd);
 
-    t2 = dup(cwfd);
+    t2 = dupOrExit(cwfd);
 
-    t3 = dup(fileno(debug_log));
+    t3 = dupOrExit(fileno(debug_log));
 
     assert(t1 > 2 && t2 > 2 && t3 > 2);