From: Amos Jeffries <yadij@users.noreply.github.com>
Date: Sun, 1 Aug 2021 12:58:25 +0000 (+0000)
Subject: Fix TCP keepalive (#853)
X-Git-Tag: SQUID_6_0_1~303
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1dde46601ec02f0e9879d6d37199a1353a88c860;p=thirdparty%2Fsquid.git

Fix TCP keepalive (#853)

Setting TCP keep-alive flags at accept(2) time resolves issues with
client sockets timing out while waiting for the ::Server handler to run.

Also resolves a bug with FTP DATA connections not having keep-alive set.
These connections would truncate objects if the data transfer connection
paused for too long and became timed out by the network routing system.
---

diff --git a/acinclude/ax_cxx_0x_types.m4 b/acinclude/ax_cxx_0x_types.m4
index 52ab37b21f..289b49f53f 100644
--- a/acinclude/ax_cxx_0x_types.m4
+++ b/acinclude/ax_cxx_0x_types.m4
@@ -57,3 +57,22 @@ enum class testEnum { one, two, three };
      [$squid_cv_have_std_underlying_type],
      [Define if stdlibc support std::underlying_type for enums])
 ])
+
+## SQUID_CXX_STD_IS_TRIVIALLY_COPYABLE
+## checks whether the std::is_trivially_copyable<> trait exists
+## (known to be missing in GCC until version 5.1)
+AC_DEFUN([SQUID_CXX_STD_IS_TRIVIALLY_COPYABLE],[
+  AC_CACHE_CHECK([whether compiler supports std::is_trivially_copyable],
+    [squid_cv_have_std_is_trivially_copyable],[
+      AC_REQUIRE([AC_PROG_CXX])
+      AC_LANG_PUSH([C++])
+      AC_TRY_COMPILE([#include <type_traits>],
+        [return std::is_trivially_copyable<int>::value ? 1 : 0;],
+        [squid_cv_have_std_is_trivially_copyable=yes],
+        [squid_cv_have_std_is_trivially_copyable=no])
+      AC_LANG_POP
+  ])
+  SQUID_DEFINE_BOOL([HAVE_STD_IS_TRIVIALLY_COPYABLE],
+     [$squid_cv_have_std_is_trivially_copyable],
+     [Define if stdlibc support std::is_trivially_copyable])
+])
diff --git a/configure.ac b/configure.ac
index 3ce45e1f36..dcdfa45cd7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2900,6 +2900,7 @@ AC_CHECK_SIZEOF(size_t)
 dnl Some C++11 types we try to use
 AX_CXX_TYPE_UNIFORM_DISTRIBUTIONS
 SQUID_CXX_STD_UNDERLYING_TYPE
+SQUID_CXX_STD_IS_TRIVIALLY_COPYABLE
 
 dnl On Solaris 9 x86, gcc may includes a "fixed" set of old system include files
 dnl that is incompatible with the updated Solaris header files.
diff --git a/src/anyp/PortCfg.cc b/src/anyp/PortCfg.cc
index 95d12be739..6a86396b5c 100644
--- a/src/anyp/PortCfg.cc
+++ b/src/anyp/PortCfg.cc
@@ -42,7 +42,6 @@ AnyP::PortCfg::PortCfg() :
     workerQueues(false),
     listenConn()
 {
-    memset(&tcp_keepalive, 0, sizeof(tcp_keepalive));
 }
 
 AnyP::PortCfg::~PortCfg()
diff --git a/src/anyp/PortCfg.h b/src/anyp/PortCfg.h
index 7c46a85b56..5cffac33a5 100644
--- a/src/anyp/PortCfg.h
+++ b/src/anyp/PortCfg.h
@@ -14,6 +14,7 @@
 #include "anyp/TrafficMode.h"
 #include "base/CodeContext.h"
 #include "comm/Connection.h"
+#include "comm/Tcp.h"
 #include "sbuf/SBuf.h"
 #include "security/ServerOptions.h"
 
@@ -53,12 +54,7 @@ public:
     int disable_pmtu_discovery;
     bool workerQueues; ///< whether listening queues should be worker-specific
 
-    struct {
-        unsigned int idle;
-        unsigned int interval;
-        unsigned int timeout;
-        bool enabled;
-    } tcp_keepalive;
+    Comm::TcpKeepAlive tcp_keepalive;
 
     /**
      * The listening socket details.
diff --git a/src/client_side.cc b/src/client_side.cc
index 8e1659a028..7da4c82925 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2328,9 +2328,6 @@ httpAccept(const CommAcceptCbParams &params)
     debugs(33, 4, params.conn << ": accepted");
     fd_note(params.conn->fd, "client http connect");
 
-    if (s->tcp_keepalive.enabled)
-        commSetTcpKeepalive(params.conn->fd, s->tcp_keepalive.idle, s->tcp_keepalive.interval, s->tcp_keepalive.timeout);
-
     // Socket is ready, setup the connection manager to start using it
     auto *srv = Http::NewServer(xact);
     AsyncJob::Start(srv); // usually async-calls readSomeData()
@@ -2531,10 +2528,6 @@ httpsAccept(const CommAcceptCbParams &params)
     debugs(33, 4, HERE << params.conn << " accepted, starting SSL negotiation.");
     fd_note(params.conn->fd, "client https connect");
 
-    if (s->tcp_keepalive.enabled) {
-        commSetTcpKeepalive(params.conn->fd, s->tcp_keepalive.idle, s->tcp_keepalive.interval, s->tcp_keepalive.timeout);
-    }
-
     // Socket is ready, setup the connection manager to start using it
     auto *srv = Https::NewServer(xact);
     AsyncJob::Start(srv); // usually async-calls postHttpsAccept()
diff --git a/src/comm.cc b/src/comm.cc
index 60d4f1b39f..dba7c60e9c 100644
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -1142,41 +1142,6 @@ commSetTcpNoDelay(int fd)
 
 #endif
 
-void
-commSetTcpKeepalive(int fd, int idle, int interval, int timeout)
-{
-    int on = 1;
-#ifdef TCP_KEEPCNT
-    if (timeout && interval) {
-        int count = (timeout + interval - 1) / interval;
-        if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &count, sizeof(on)) < 0) {
-            int xerrno = errno;
-            debugs(5, DBG_IMPORTANT, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-        }
-    }
-#endif
-#ifdef TCP_KEEPIDLE
-    if (idle) {
-        if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &idle, sizeof(on)) < 0) {
-            int xerrno = errno;
-            debugs(5, DBG_IMPORTANT, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-        }
-    }
-#endif
-#ifdef TCP_KEEPINTVL
-    if (interval) {
-        if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &interval, sizeof(on)) < 0) {
-            int xerrno = errno;
-            debugs(5, DBG_IMPORTANT, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-        }
-    }
-#endif
-    if (setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (char *) &on, sizeof(on)) < 0) {
-        int xerrno = errno;
-        debugs(5, DBG_IMPORTANT, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-    }
-}
-
 void
 comm_init(void)
 {
diff --git a/src/comm.h b/src/comm.h
index 2e9dea96fa..c27fe01045 100644
--- a/src/comm.h
+++ b/src/comm.h
@@ -23,7 +23,6 @@ bool comm_iocallbackpending(void); /* inline candidate */
 int commSetNonBlocking(int fd);
 int commUnsetNonBlocking(int fd);
 void commSetCloseOnExec(int fd);
-void commSetTcpKeepalive(int fd, int idle, int interval, int timeout);
 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/Makefile.am b/src/comm/Makefile.am
index 0c5d24e904..a5c933d608 100644
--- a/src/comm/Makefile.am
+++ b/src/comm/Makefile.am
@@ -30,6 +30,8 @@ libcomm_la_SOURCES = \
 	ModSelectWin32.cc \
 	Read.cc \
 	Read.h \
+	Tcp.cc \
+	Tcp.h \
 	TcpAcceptor.cc \
 	TcpAcceptor.h \
 	UdpOpenDialer.h \
diff --git a/src/comm/Tcp.cc b/src/comm/Tcp.cc
new file mode 100644
index 0000000000..59769a3233
--- /dev/null
+++ b/src/comm/Tcp.cc
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 1996-2021 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.
+ */
+
+/* DEBUG: section 05    TCP Socket Functions */
+
+#include "squid.h"
+#include "comm/Tcp.h"
+#include "Debug.h"
+
+#if HAVE_NETINET_TCP_H
+#include <netinet/tcp.h>
+#endif
+#include <type_traits>
+
+/// setsockopt(2) wrapper
+template <typename Option>
+static bool
+SetSocketOption(const int fd, const int level, const int optName, const Option &optValue)
+{
+#if HAVE_STD_IS_TRIVIALLY_COPYABLE
+    static_assert(std::is_trivially_copyable<Option>::value, "setsockopt() expects POD-like options");
+#endif
+    static_assert(!std::is_same<Option, bool>::value, "setsockopt() uses int to represent boolean options");
+    if (setsockopt(fd, level, optName, &optValue, sizeof(optValue)) < 0) {
+        const auto xerrno = errno;
+        debugs(5, DBG_IMPORTANT, "ERROR: setsockopt(2) failure: " << xstrerr(xerrno));
+        // TODO: Generalize to throw on errors when some callers need that.
+        return false;
+    }
+    return true;
+}
+
+/// setsockopt(2) wrapper for setting typical on/off options
+static bool
+SetBooleanSocketOption(const int fd, const int level, const int optName, const bool enable)
+{
+    const int optValue = enable ? 1 : 0;
+    return SetSocketOption(fd, level, optName, optValue);
+}
+
+void
+Comm::ApplyTcpKeepAlive(int fd, const TcpKeepAlive &cfg)
+{
+    if (!cfg.enabled)
+        return;
+
+#if defined(TCP_KEEPCNT)
+    if (cfg.timeout && cfg.interval) {
+        const int count = (cfg.timeout + cfg.interval - 1) / cfg.interval; // XXX: unsigned-to-signed conversion
+        (void)SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPCNT, count);
+    }
+#endif
+#if defined(TCP_KEEPIDLE)
+    if (cfg.idle) {
+        // XXX: TCP_KEEPIDLE expects an int; cfg.idle is unsigned
+        (void)SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPIDLE, cfg.idle);
+    }
+#endif
+#if defined(TCP_KEEPINTVL)
+    if (cfg.interval) {
+        // XXX: TCP_KEEPINTVL expects an int; cfg.interval is unsigned
+        (void)SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPINTVL, cfg.interval);
+    }
+#endif
+    (void)SetBooleanSocketOption(fd, SOL_SOCKET, SO_KEEPALIVE, true);
+}
diff --git a/src/comm/Tcp.h b/src/comm/Tcp.h
new file mode 100644
index 0000000000..94508d2c3d
--- /dev/null
+++ b/src/comm/Tcp.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 1996-2021 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_COMM_TCP_H
+#define SQUID__SRC_COMM_TCP_H
+
+namespace Comm
+{
+
+/// Configuration settings for the TCP keep-alive feature
+class TcpKeepAlive
+{
+public:
+    unsigned int idle = 0;
+    unsigned int interval = 0;
+    unsigned int timeout = 0;
+    bool enabled = false;
+};
+
+/// apply configured TCP keep-alive settings to the given FD socket
+void ApplyTcpKeepAlive(int fd, const TcpKeepAlive &);
+
+} // namespace Comm
+
+#endif /* SQUID__SRC_COMM_TCP_H */
diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index 17639607f7..0c3addfa5b 100644
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -430,6 +430,7 @@ Comm::TcpAcceptor::oldAccept(Comm::ConnectionPointer &details)
     // set socket flags
     commSetCloseOnExec(sock);
     commSetNonBlocking(sock);
+    Comm::ApplyTcpKeepAlive(sock, listenPort_->tcp_keepalive);
 
     /* IFF the socket is (tproxy) transparent, pass the flag down to allow spoofing */
     F->flags.transparent = fd_table[conn->fd].flags.transparent; // XXX: can we remove this line yet?
diff --git a/src/comm/forward.h b/src/comm/forward.h
index 126a935067..51ffb5a3fd 100644
--- a/src/comm/forward.h
+++ b/src/comm/forward.h
@@ -23,6 +23,7 @@ namespace Comm
 
 class Connection;
 class ConnOpener;
+class TcpKeepAlive;
 
 typedef RefCount<Comm::Connection> ConnectionPointer;
 
diff --git a/src/ipc/TypedMsgHdr.h b/src/ipc/TypedMsgHdr.h
index f97fe828f2..9a60cd415a 100644
--- a/src/ipc/TypedMsgHdr.h
+++ b/src/ipc/TypedMsgHdr.h
@@ -115,8 +115,10 @@ template <class Pod>
 void
 Ipc::TypedMsgHdr::getPod(Pod &pod) const
 {
+#if HAVE_STD_IS_TRIVIALLY_COPYABLE
     // TODO: Enable after fixing Ipc::SharedListenRequest::SharedListenRequest()
     //static_assert(std::is_trivially_copyable<Pod>::value, "getPod() used for a POD");
+#endif
     getFixed(&pod, sizeof(pod));
 }
 
@@ -124,8 +126,10 @@ template <class Pod>
 void
 Ipc::TypedMsgHdr::putPod(const Pod &pod)
 {
+#if HAVE_STD_IS_TRIVIALLY_COPYABLE
     // TODO: Enable after fixing Ipc::SharedListenRequest::pack()
     //static_assert(std::is_trivially_copyable<Pod>::value, "putPod() used for a POD");
+#endif
     putFixed(&pod, sizeof(pod));
 }
 
diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index 8b015f72d1..5203d8354e 100644
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -254,9 +254,6 @@ Ftp::Server::AcceptCtrlConnection(const CommAcceptCbParams &params)
     debugs(33, 4, params.conn << ": accepted");
     fd_note(params.conn->fd, "client ftp connect");
 
-    if (s->tcp_keepalive.enabled)
-        commSetTcpKeepalive(params.conn->fd, s->tcp_keepalive.idle, s->tcp_keepalive.interval, s->tcp_keepalive.timeout);
-
     AsyncJob::Start(new Server(xact));
 }
 
diff --git a/src/tests/stub_comm.cc b/src/tests/stub_comm.cc
index 9c99de7b73..d2b15eb456 100644
--- a/src/tests/stub_comm.cc
+++ b/src/tests/stub_comm.cc
@@ -33,7 +33,6 @@ bool comm_iocallbackpending(void) STUB_RETVAL(false)
 int commSetNonBlocking(int fd) STUB_RETVAL(Comm::COMM_ERROR)
 int commUnsetNonBlocking(int fd) STUB_RETVAL(-1)
 void commSetCloseOnExec(int fd) STUB_NOP
-void commSetTcpKeepalive(int fd, int idle, int interval, int timeout) STUB
 void _comm_close(int fd, char const *file, int line) STUB
 void old_comm_reset_close(int fd) STUB
 void comm_reset_close(const Comm::ConnectionPointer &conn) STUB
diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc
index 7f3fe834b7..cf1c2d2fc3 100644
--- a/src/tests/stub_libcomm.cc
+++ b/src/tests/stub_libcomm.cc
@@ -76,6 +76,9 @@ void Comm::TcpAcceptor::unsubscribe(const char *) STUB
 void Comm::TcpAcceptor::acceptNext() STUB
 void Comm::TcpAcceptor::notify(const Comm::Flag flag, const Comm::ConnectionPointer &) const STUB
 
+#include "comm/Tcp.h"
+void Comm::ApplyTcpKeepAlive(int, const TcpKeepAlive &) STUB
+
 #include "comm/Write.h"
 void Comm::Write(const Comm::ConnectionPointer &, const char *, int, AsyncCall::Pointer &, FREE *) STUB
 void Comm::Write(const Comm::ConnectionPointer &conn, MemBuf *mb, AsyncCall::Pointer &callback) STUB