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 eb293542053038cd412de974b496e6c59d15dfa9..15860c6ccea0a021262cd6924f6b7d41c2ef3bc3 100644 (file)
--- 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 df64d9a5e238c16d64d656f955d0f46985af9092..b9d4bb967af14cb696a556bb545dbc0bde057120 100644 (file)
--- 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 aabf2e465daf08dc7b05f1ef06909deade08e5e2..9af1a42ae0d50e24a2fd684c8e11ba88b14aa3e2 100644 (file)
--- 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 1cc3266ba1657bb1af3df56ee73b429e6c4e7356..359eae23b0d0b5b75aa761cfcd70d846f3952796 100644 (file)
--- 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