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 70a784a29fda859498f7dac32c08f229306dfc51..3c367274c0dbf3747fa2d8f8c5d0ea1d1ba342e6 100644 (file)
--- 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 6a6d2f2dd358fb63b5fb99e5a9b59f521ef5aa21..685e74209c76500429a814744a56b7b2e110392f 100644 (file)
--- 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 94fc93b6a8828c0c0cf710ec787d63c6e12b9de7..e624c6a4e4ee57182d64e35a5181e96d93a944a3 100644 (file)
--- 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;