From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sun, 17 Dec 2023 14:48:41 +0000 (+0000)
Subject: Bug 5254, part 1: Do not leak master process' cache.log to kids (#1222)
X-Git-Tag: SQUID_7_0_1~250
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1a848e4ef8ecf2da8887ec57f42cdcdc56eb40eb;p=thirdparty%2Fsquid.git

Bug 5254, part 1: Do not leak master process' cache.log to kids (#1222)

The fork()ed kids unknowingly inherited cache.log file descriptor and,
hence, prevented the underlying file from being deleted on log rotation.

This change does not fully fix the bug because the file is still being
held open by the master process itself. This change was isolated because
it lacks bad/controversial side effects -- it is a simple step forward.

This fix may also help stop leaking kid's cache.log descriptor to
helpers, but that requires more work -- replacing descriptor duping
trick in ipcCreate() with a proper servicing of helper stderr descriptor
via Comm. For now, each helper process still keeps cache.log alive.
---

diff --git a/src/comm.h b/src/comm.h
index 70a784a29f..3c367274c0 100644
--- a/src/comm.h
+++ b/src/comm.h
@@ -22,7 +22,16 @@ bool comm_iocallbackpending(void); /* inline candidate */
 
 int commSetNonBlocking(int fd);
 int commUnsetNonBlocking(int fd);
+
+/// On platforms where FD_CLOEXEC is defined, close the given descriptor during
+/// a function call from the exec(3) family. Otherwise, do nothing; the platform
+/// itself may close-on-exec by default (e.g., MS Win32 is said to do that at
+/// https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873); other
+/// platforms are unsupported. Callers that want close-on-exec behavior must
+/// call this function on all platforms and are not responsible for the outcome
+/// on platforms without FD_CLOEXEC.
 void commSetCloseOnExec(int fd);
+
 void _comm_close(int fd, char const *file, int line);
 #define comm_close(x) (_comm_close((x), __FILE__, __LINE__))
 void old_comm_reset_close(int fd);
diff --git a/src/comm/minimal.cc b/src/comm/minimal.cc
index 6a6d2f2dd3..685e74209c 100644
--- a/src/comm/minimal.cc
+++ b/src/comm/minimal.cc
@@ -7,6 +7,7 @@
  */
 
 #include "squid.h"
+#include "comm.h"
 #include "debug/Stream.h"
 #include "fd.h"
 
@@ -22,3 +23,12 @@ fd_close(const int fd)
     debugs(51, 3, "FD " << fd);
 }
 
+void
+commSetCloseOnExec(int)
+{
+    // This stub is needed because DebugFile sets this flag for the open
+    // cache.log file descriptor. Helpers and such must use stdout/stderr
+    // instead of opening a cache.log file. They should never reach this code.
+    assert(false);
+}
+
diff --git a/src/debug/debug.cc b/src/debug/debug.cc
index 94fc93b6a8..e624c6a4e4 100644
--- a/src/debug/debug.cc
+++ b/src/debug/debug.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "base/TextException.h"
+#include "comm.h"
 #include "debug/Stream.h"
 #include "fatal.h"
 #include "fd.h"
@@ -762,8 +763,10 @@ DebugFile::reset(FILE *newFile, const char *newName)
     }
     file_ = newFile; // may be nil
 
-    if (file_)
+    if (file_) {
+        commSetCloseOnExec(fileno(file_));
         fd_open(fileno(file_), FD_LOG, Debug::cache_log);
+    }
 
     xfree(name);
     name = newName ? xstrdup(newName) : nullptr;