From 7224fa1b9613310c56a5f8643f49f616c7fb2da2 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Sun, 10 Nov 2013 15:58:31 -0700 Subject: [PATCH] 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 --- src/ipc/TypedMsgHdr.cc | 13 +++++++++++++ src/ipc/TypedMsgHdr.h | 3 ++- src/snmp/Inquirer.cc | 4 ++++ src/snmp/Request.cc | 7 +++++-- 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/ipc/TypedMsgHdr.cc b/src/ipc/TypedMsgHdr.cc index 74cae6aa7a..924507834f 100644 --- a/src/ipc/TypedMsgHdr.cc +++ b/src/ipc/TypedMsgHdr.cc @@ -167,10 +167,20 @@ Ipc::TypedMsgHdr::putRaw(const void *raw, size_t size) } } +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(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 7b9b882455..cb2dc1412a 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(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.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 -- 2.47.2