------------------------------------------------------------
revno: 12650
revision-id: squid3@treenet.co.nz-20131110225831-v3p8p8ssyetvxnxc
parent: squid3@treenet.co.nz-20131103110406-7ifa3zcvprqt2zlx
fixes bug(s): http://bugs.squid-cache.org/show_bug.cgi?id=3906
author: Christos Tsantilas <chtsanti@users.sourceforge.net>, Alex Rousskov <rousskov@measurement-factory.com>
committer: Amos Jeffries <squid3@treenet.co.nz>
branch nick: 3.3
timestamp: Sun 2013-11-10 15:58:31 -0700
message:
  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
------------------------------------------------------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: squid3@treenet.co.nz-20131110225831-v3p8p8ssyetvxnxc
# target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# testament_sha1: 425d24ff2998e89cad3a720acc8e7c2fa47b25d1
# timestamp: 2013-11-10 23:56:59 +0000
# source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# base_revision_id: squid3@treenet.co.nz-20131103110406-\
#   7ifa3zcvprqt2zlx
# 
# Begin patch
=== modified file 'src/ipc/TypedMsgHdr.cc'
--- src/ipc/TypedMsgHdr.cc	2012-12-02 07:23:32 +0000
+++ src/ipc/TypedMsgHdr.cc	2013-11-10 22:58:31 +0000
@@ -167,10 +167,20 @@
     }
 }
 
+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 @@
     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);

=== modified file 'src/ipc/TypedMsgHdr.h'
--- src/ipc/TypedMsgHdr.h	2012-09-01 14:38:36 +0000
+++ src/ipc/TypedMsgHdr.h	2013-11-10 22:58:31 +0000
@@ -59,7 +59,8 @@
 
     /* 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

=== modified file 'src/snmp/Inquirer.cc'
--- src/snmp/Inquirer.cc	2012-09-01 14:38:36 +0000
+++ src/snmp/Inquirer.cc	2013-11-10 22:58:31 +0000
@@ -28,6 +28,10 @@
     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

=== modified file 'src/snmp/Request.cc'
--- src/snmp/Request.cc	2012-09-01 14:38:36 +0000
+++ src/snmp/Request.cc	2013-11-10 22:58:31 +0000
@@ -33,7 +33,8 @@
     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 @@
     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