From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Wed, 6 Nov 2013 18:11:29 +0000 (+0200)
Subject: Bug 3906: Filedescriptor leaks on snmp
X-Git-Tag: SQUID_3_5_0_1~533
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b4405598cb173b16684b9efdec6568d92199b02;p=thirdparty%2Fsquid.git

Bug 3906: Filedescriptor leaks on snmp
author: Alex Rousskov <rousskov@measurement-factory.com>, Christos Tsantilas <chtsanti@users.sourceforge.net>

Coordinator should not send SNMP client FD to strands when broadcasting SNMP
requests. Strands do not need the descriptor and were forgetting to close it,
causing one FD leak on every SNMP query in SMP mode.

Enhance Ipc::TypedMsgHdr to be able to tell whether the message has a FD.

This is a Measurement Factory project
---

diff --git a/src/ipc/TypedMsgHdr.cc b/src/ipc/TypedMsgHdr.cc
index eb29354205..15860c6cce 100644
--- a/src/ipc/TypedMsgHdr.cc
+++ b/src/ipc/TypedMsgHdr.cc
@@ -167,10 +167,20 @@ Ipc::TypedMsgHdr::putRaw(const void *rawBuf, size_t rawSize)
     }
 }
 
+bool
+Ipc::TypedMsgHdr::hasFd() const
+{
+     struct cmsghdr *cmsg = CMSG_FIRSTHDR(this);
+     return cmsg &&
+         cmsg->cmsg_level == SOL_SOCKET &&
+         cmsg->cmsg_type == SCM_RIGHTS;
+}
+
 void
 Ipc::TypedMsgHdr::putFd(int fd)
 {
     Must(fd >= 0);
+    Must(!hasFd());
     allocControl();
 
     const int fdCount = 1;
@@ -183,12 +193,15 @@ Ipc::TypedMsgHdr::putFd(int fd)
     int *fdStore = reinterpret_cast<int*>(CMSG_DATA(cmsg));
     memcpy(fdStore, &fd, fdCount * sizeof(int));
     msg_controllen = cmsg->cmsg_len;
+
+    Must(hasFd());
 }
 
 int
 Ipc::TypedMsgHdr::getFd() const
 {
     Must(msg_control && msg_controllen);
+    Must(hasFd());
 
     struct cmsghdr *cmsg = CMSG_FIRSTHDR(this);
     Must(cmsg->cmsg_level == SOL_SOCKET);
diff --git a/src/ipc/TypedMsgHdr.h b/src/ipc/TypedMsgHdr.h
index df64d9a5e2..b9d4bb967a 100644
--- a/src/ipc/TypedMsgHdr.h
+++ b/src/ipc/TypedMsgHdr.h
@@ -59,7 +59,8 @@ public:
 
     /* access to a "file" descriptor that can be passed between processes */
     void putFd(int aFd); ///< stores descriptor
-    int getFd() const; ///< returns descriptor
+    int getFd() const; ///< returns stored descriptor
+    bool hasFd() const; ///< whether the message has a descriptor stored
 
     /* raw, type-independent access for I/O */
     void prepForReading(); ///< reset and provide all buffers
diff --git a/src/snmp/Inquirer.cc b/src/snmp/Inquirer.cc
index aabf2e465d..9af1a42ae0 100644
--- a/src/snmp/Inquirer.cc
+++ b/src/snmp/Inquirer.cc
@@ -28,6 +28,10 @@ Snmp::Inquirer::Inquirer(const Request& aRequest, const Ipc::StrandCoords& coord
     closer = asyncCall(49, 5, "Snmp::Inquirer::noteCommClosed",
                        CommCbMemFunT<Inquirer, CommCloseCbParams>(this, &Inquirer::noteCommClosed));
     comm_add_close_handler(conn->fd, closer);
+
+    // forget client FD to avoid sending it to strands that may forget to close
+    if (Request *snmpRequest = dynamic_cast<Request*>(request.getRaw()))
+        snmpRequest->fd = -1;
 }
 
 /// closes our copy of the client connection socket
diff --git a/src/snmp/Request.cc b/src/snmp/Request.cc
index 1cc3266ba1..359eae23b0 100644
--- a/src/snmp/Request.cc
+++ b/src/snmp/Request.cc
@@ -33,7 +33,8 @@ Snmp::Request::Request(const Ipc::TypedMsgHdr& msg):
     session.unpack(msg);
     msg.getPod(address);
 
-    fd = msg.getFd();
+    // Requests from strands have FDs. Requests from Coordinator do not.
+    fd = msg.hasFd() ? msg.getFd() : -1;
 }
 
 void
@@ -46,7 +47,9 @@ Snmp::Request::pack(Ipc::TypedMsgHdr& msg) const
     session.pack(msg);
     msg.putPod(address);
 
-    msg.putFd(fd);
+    // Requests sent to Coordinator have FDs. Requests sent to strands do not.
+    if (fd >= 0)
+        msg.putFd(fd);
 }
 
 Ipc::Request::Pointer