From: Eduard Bagdasaryan Date: Fri, 8 Nov 2024 13:23:28 +0000 (+0000) Subject: Reduce SO_LINGER code duplication (#1941) X-Git-Tag: SQUID_7_0_1~41 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a2ddc0b710f6b4c53b0648feed1f09bd9f92b08d;p=thirdparty%2Fsquid.git 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 ba18de1f8c..ea6fd6e63f 100644 --- 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 index 0000000000..defe41370c --- /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 5c8fbd83f1..c496b8aaa3 100644 --- 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(&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 7607c99ba0..930edc08ed 100644 --- 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 f80df84cd3..492bfea5ef 100644 --- 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;