Reduce SO_LINGER code duplication (#1941)

Also prevent nil connection pointer dereference and setsockopt() calls
with negative FD in comm_reset_close() and old_comm_reset_close().
It is unknown whether such bugs could be triggered before these changes.

Also removed fde::flags::nolinger as unused since 1999 commit 2391a162.

diff --git a/src/base/Makefile.am b/src/base/Makefile.am
index ba18de1f8cc652e77afe1454185f48b464c9d4fa..ea6fd6e63fd49450a657af46abc64ee63f8ee3f9 100644 (file)
--- a/src/base/Makefile.am
+++ b/src/base/Makefile.am
@@ -48,6 +48,7 @@ libbase_la_SOURCES = \
        JobWait.h \
        Lock.h \
        LookupTable.h \
+       OnOff.h \
        Packable.h \
        PackableStream.h \
        Random.cc \

diff --git a/src/base/OnOff.h b/src/base/OnOff.h
new file mode 100644 (file)
index 0000000..defe413
--- /dev/null
+++ b/src/base/OnOff.h
@@ -0,0 +1,16 @@
+/*
+ * Copyright (C) 1996-2024 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_SRC_BASE_ONOFF_H
+#define SQUID_SRC_BASE_ONOFF_H
+
+/// safer than bool in a list of integer-like function parameters
+enum class OnOff { off, on };
+
+#endif /* SQUID_SRC_BASE_ONOFF_H */
+

diff --git a/src/comm.cc b/src/comm.cc
index 5c8fbd83f15f1c01d5cdd2c16db9ecd710281927..c496b8aaa3aa7d49340de9754fff97740d74f575 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "base/AsyncFunCalls.h"
+#include "base/OnOff.h"
 #include "ClientInfo.h"
 #include "comm/AcceptLimiter.h"
 #include "comm/comm_internal.h"
@@ -78,7 +79,7 @@ static void commPlanHalfClosedCheck();
 static Comm::Flag commBind(int s, struct addrinfo &);
 static void commSetBindAddressNoPort(int);
 static void commSetReuseAddr(int);
-static void commSetNoLinger(int);
+static void commConfigureLinger(int fd, OnOff);
 #ifdef TCP_NODELAY
 static void commSetTcpNoDelay(int);
 #endif
@@ -485,7 +486,7 @@ comm_apply_flags(int new_socket,
 #if _SQUID_WINDOWS_
         if (sock_type != SOCK_DGRAM)
 #endif
-            commSetNoLinger(new_socket);
+            commConfigureLinger(new_socket, OnOff::off);
 
         if (opt_reuseaddr)
             commSetReuseAddr(new_socket);
@@ -555,13 +556,6 @@ comm_import_opened(const Comm::ConnectionPointer &conn,
 
     comm_init_opened(conn, note, AI);
 
-    if (conn->local.port() > (unsigned short) 0) {
-#if _SQUID_WINDOWS_
-        if (AI->ai_socktype != SOCK_DGRAM)
-#endif
-            fd_table[conn->fd].flags.nolinger = true;
-    }
-
     if ((conn->flags & COMM_TRANSPARENT))
         fd_table[conn->fd].flags.transparent = true;
 
@@ -780,6 +774,21 @@ commCallCloseHandlers(int fd)
     }
 }
 
+/// sets SO_LINGER socket(7) option
+/// \param enabled -- whether linger will be active (sets linger::l_onoff)
+static void
+commConfigureLinger(const int fd, const OnOff enabled)
+{
+    struct linger l = {};
+    l.l_onoff = (enabled == OnOff::on ? 1 : 0);
+    l.l_linger = 0; // how long to linger for, in seconds
+
+    if (setsockopt(fd, SOL_SOCKET, SO_LINGER, reinterpret_cast<char*>(&l), sizeof(l)) < 0) {
+        const auto xerrno = errno;
+        debugs(50, DBG_CRITICAL, "ERROR: Failed to set closure behavior (SO_LINGER) for FD " << fd << ": " << xstrerr(xerrno));
+    }
+}
+
 /**
  * enable linger with time of 0 so that when the socket is
  * closed, TCP generates a RESET
@@ -787,30 +796,21 @@ commCallCloseHandlers(int fd)
 void
 comm_reset_close(const Comm::ConnectionPointer &conn)
 {
-    struct linger L;
-    L.l_onoff = 1;
-    L.l_linger = 0;
-
-    if (setsockopt(conn->fd, SOL_SOCKET, SO_LINGER, (char *) &L, sizeof(L)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_CRITICAL, "ERROR: Closing " << conn << " with TCP RST: " << xstrerr(xerrno));
+    if (Comm::IsConnOpen(conn)) {
+        commConfigureLinger(conn->fd, OnOff::on);
+        debugs(5, 7, conn->id);
+        conn->close();
     }
-    conn->close();
 }
 
 // Legacy close function.
 void
 old_comm_reset_close(int fd)
 {
-    struct linger L;
-    L.l_onoff = 1;
-    L.l_linger = 0;
-
-    if (setsockopt(fd, SOL_SOCKET, SO_LINGER, (char *) &L, sizeof(L)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_CRITICAL, "ERROR: Closing FD " << fd << " with TCP RST: " << xstrerr(xerrno));
+    if (fd >= 0) {
+        commConfigureLinger(fd, OnOff::on);
+        comm_close(fd);
     }
-    comm_close(fd);
 }
 
 static void
@@ -1021,21 +1021,6 @@ comm_remove_close_handler(int fd, AsyncCall::Pointer &call)
     call->cancel("comm_remove_close_handler");
 }
 
-static void
-commSetNoLinger(int fd)
-{
-
-    struct linger L;
-    L.l_onoff = 0;      /* off */
-    L.l_linger = 0;
-
-    if (setsockopt(fd, SOL_SOCKET, SO_LINGER, (char *) &L, sizeof(L)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_CRITICAL, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-    }
-    fd_table[fd].flags.nolinger = true;
-}
-
 static void
 commSetReuseAddr(int fd)
 {

diff --git a/src/fde.h b/src/fde.h
index 7607c99ba0d8d586d8c10ae82dc76ad9131d3f83..930edc08ed43e0a46910d21692ceb2a9fecac5a3 100644 (file)
--- a/src/fde.h
+++ b/src/fde.h
@@ -119,7 +119,6 @@ public:
         bool close_request = false; ///< true if file_ or comm_close has been called
         bool write_daemon = false;
         bool socket_eof = false;
-        bool nolinger = false;
         bool nonblocking = false;
         bool ipc = false;
         bool called_connect = false;

diff --git a/src/ipc_win32.cc b/src/ipc_win32.cc
index f80df84cd3330666c8c20137775ba758bfd9f9b7..492bfea5efa6b47135c82411046519e8935b5230 100644 (file)
--- a/src/ipc_win32.cc
+++ b/src/ipc_win32.cc
@@ -674,7 +674,7 @@ ipc_thread_1(void *in_params)
     }
 
     /* else {                       IPC_TCP_SOCKET */
-    /*     commSetNoLinger(fd); */
+    /*     commConfigureLinger(fd, OnOff::off); */
     /*  } */
     thread_params.prog = prog;