Fix TCP keepalive (squid-cache#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.

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])
+])

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.

src/anyp/PortCfg.cc

@@ -42,7 +42,6 @@ AnyP::PortCfg::PortCfg() :
     workerQueues(false),
     listenConn()
 {
-    memset(&tcp_keepalive, 0, sizeof(tcp_keepalive));
 }
 
 AnyP::PortCfg::~PortCfg()

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 @@ class PortCfg : public CodeContext
     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.

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()

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)
 {

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);

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 \

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);
+}

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 */

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?

src/comm/forward.h

@@ -23,6 +23,7 @@ namespace Comm
 
 class Connection;
 class ConnOpener;
+class TcpKeepAlive;
 
 typedef RefCount<Comm::Connection> ConnectionPointer;
 

src/ipc/TypedMsgHdr.h

@@ -115,17 +115,21 @@ 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));
 }
 
 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));
 }
 

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));
 }
 

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

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