Bug 3906: Filedescriptor leaks in SNMP

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 7b9b882455f2f005ce91d71d1a44ca39aa77a8af..cb2dc1412afd91da13f278824f6fc816182fc88f 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