From a7b68ae7e414ec9f3184df70ac8008f8a310ae60 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Mon, 5 Dec 2016 16:42:55 +0100
Subject: [PATCH] Handle exceptions raised by `closesocket()`

This was not very well handled, and could cause the PowerDNS process
to terminate. This is especially nasty when `closesocket()` is called
from a destructor, as we could already be dealing with an exception.
---
 pdns/dnsproxy.cc       |  9 +++++++-
 pdns/pdns_recursor.cc  | 46 +++++++++++++++++++++++++++++++++------
 pdns/resolver.cc       | 19 +++++++++++++---
 pdns/rfc2136handler.cc | 49 ++++++++++++++++++++++++++++++++++++------
 pdns/sstuff.hh         |  7 +++++-
 pdns/tcpreceiver.cc    | 15 +++++++++++--
 pdns/utility.hh        |  3 ---
 7 files changed, 124 insertions(+), 24 deletions(-)

diff --git a/pdns/dnsproxy.cc b/pdns/dnsproxy.cc
index 280fc127c4..0c300b086f 100644
--- a/pdns/dnsproxy.cc
+++ b/pdns/dnsproxy.cc
@@ -303,6 +303,13 @@ void DNSProxy::mainloop(void)
 }
 
 DNSProxy::~DNSProxy() {
-  if (d_sock>-1) closesocket(d_sock);
+  if (d_sock>-1) {
+    try {
+      closesocket(d_sock);
+    }
+    catch(const PDNSException& e) {
+    }
+  }
+
   d_sock=-1;
 }
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index e89b1855b0..51457f8068 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -384,7 +384,13 @@ public:
     if(connect(*fd, (struct sockaddr*)(&toaddr), toaddr.getSocklen()) < 0) {
       int err = errno;
       //      returnSocket(*fd);
-      closesocket(*fd);
+      try {
+        closesocket(*fd);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing UDP socket after connect() failed: "<<e.reason<<endl;
+      }
+
       if(err==ENETUNREACH) // Seth "My Interfaces Are Like A Yo Yo" Arnold special
         return -2;
       return -1;
@@ -416,7 +422,12 @@ public:
     catch(FDMultiplexerException& e) {
       // we sometimes return a socket that has not yet been assigned to t_fdm
     }
-    closesocket(*i);
+    try {
+      closesocket(*i);
+    }
+    catch(const PDNSException& e) {
+      L<<Logger::Error<<"Error closing returned UDP socket: "<<e.reason<<endl;
+    }
 
     d_socks.erase(i++);
     --d_numsocks;
@@ -567,8 +578,14 @@ TCPConnection::TCPConnection(int fd, const ComboAddress& addr) : d_remote(addr),
 
 TCPConnection::~TCPConnection()
 {
-  if(closesocket(d_fd) < 0)
-    unixDie("closing socket for TCPConnection");
+  try {
+    if(closesocket(d_fd) < 0)
+      L<<Logger::Error<<"Error closing socket for TCPConnection"<<endl;
+  }
+  catch(const PDNSException& e) {
+    L<<Logger::Error<<"Error closing TCPConnection socket: "<<e.reason<<endl;
+  }
+
   if(t_tcpClientCounts->count(d_remote) && !(*t_tcpClientCounts)[d_remote]--)
     t_tcpClientCounts->erase(d_remote);
   --s_currentConnections;
@@ -1405,7 +1422,12 @@ void handleNewTCPQuestion(int fd, FDMultiplexer::funcparam_t& )
   if(newsock>=0) {
     if(MT->numProcesses() > g_maxMThreads) {
       g_stats.overCapacityDrops++;
-      closesocket(newsock);
+      try {
+        closesocket(newsock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing TCP socket after an over capacity drop: "<<e.reason<<endl;
+      }
       return;
     }
 
@@ -1416,12 +1438,22 @@ void handleNewTCPQuestion(int fd, FDMultiplexer::funcparam_t& )
         L<<Logger::Error<<"["<<MT->getTid()<<"] dropping TCP query from "<<addr.toString()<<", address not matched by allow-from"<<endl;
 
       g_stats.unauthorizedTCP++;
-      closesocket(newsock);
+      try {
+        closesocket(newsock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing TCP socket after an ACL drop: "<<e.reason<<endl;
+      }
       return;
     }
     if(g_maxTCPPerClient && t_tcpClientCounts->count(addr) && (*t_tcpClientCounts)[addr] >= g_maxTCPPerClient) {
       g_stats.tcpClientOverflow++;
-      closesocket(newsock); // don't call TCPConnection::closeAndCleanup here - did not enter it in the counts yet!
+      try {
+        closesocket(newsock); // don't call TCPConnection::closeAndCleanup here - did not enter it in the counts yet!
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing TCP socket after an overflow drop: "<<e.reason<<endl;
+      }
       return;
     }
 
diff --git a/pdns/resolver.cc b/pdns/resolver.cc
index f37f2eaeca..590734a852 100644
--- a/pdns/resolver.cc
+++ b/pdns/resolver.cc
@@ -583,8 +583,14 @@ void AXFRRetriever::connect()
   int err;
 
   if((err=::connect(d_sock,(struct sockaddr*)&d_remote, d_remote.getSocklen()))<0 && errno!=EINPROGRESS) {
-    closesocket(d_sock);
-    d_sock=-1;
+    try {
+      closesocket(d_sock);
+    }
+    catch(const PDNSException& e) {
+      d_sock=-1;
+      throw ResolverException("Error closing AXFR socket after connect() failed: "+e.reason);
+    }
+
     throw ResolverException("connect: "+stringerror());
   }
 
@@ -594,7 +600,14 @@ void AXFRRetriever::connect()
   err=waitForRWData(d_sock, false, 10, 0); // wait for writeability
   
   if(!err) {
-    closesocket(d_sock); // timeout
+    try {
+      closesocket(d_sock); // timeout
+    }
+    catch(const PDNSException& e) {
+      d_sock=-1;
+      throw ResolverException("Error closing AXFR socket after timeout: "+e.reason);
+    }
+
     d_sock=-1;
     errno=ETIMEDOUT;
     
diff --git a/pdns/rfc2136handler.cc b/pdns/rfc2136handler.cc
index 9c08f8f8b3..34042a8341 100644
--- a/pdns/rfc2136handler.cc
+++ b/pdns/rfc2136handler.cc
@@ -603,7 +603,12 @@ int PacketHandler::forwardPacket(const string &msgPrefix, DNSPacket *p, DomainIn
 
     if( connect(sock, (struct sockaddr*)&remote, remote.getSocklen()) < 0 ) {
       L<<Logger::Error<<msgPrefix<<"Failed to connect to "<<remote.toStringWithPort()<<": "<<stringerror()<<endl;
-      closesocket(sock);
+      try {
+        closesocket(sock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing master forwarding socket after connect() failed: "<<e.reason<<endl;
+      }
       continue;
     }
 
@@ -615,19 +620,34 @@ int PacketHandler::forwardPacket(const string &msgPrefix, DNSPacket *p, DomainIn
     buffer.append(forwardPacket.getString());
     if(write(sock, buffer.c_str(), buffer.length()) < 0) {
       L<<Logger::Error<<msgPrefix<<"Unable to forward update message to "<<remote.toStringWithPort()<<", error:"<<stringerror()<<endl;
-      closesocket(sock);
+      try {
+        closesocket(sock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing master forwarding socket after write() failed: "<<e.reason<<endl;
+      }
       continue;
     }
 
     int res = waitForData(sock, 10, 0);
     if (!res) {
       L<<Logger::Error<<msgPrefix<<"Timeout waiting for reply from master at "<<remote.toStringWithPort()<<endl;
-      closesocket(sock);
+      try {
+        closesocket(sock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing master forwarding socket after a timeout occured: "<<e.reason<<endl;
+      }
       continue;
     }
     if (res < 0) {
       L<<Logger::Error<<msgPrefix<<"Error waiting for answer from master at "<<remote.toStringWithPort()<<", error:"<<stringerror()<<endl;
-      closesocket(sock);
+      try {
+        closesocket(sock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing master forwarding socket after an error occured: "<<e.reason<<endl;
+      }
       continue;
     }
 
@@ -636,7 +656,12 @@ int PacketHandler::forwardPacket(const string &msgPrefix, DNSPacket *p, DomainIn
     recvRes = recv(sock, &lenBuf, sizeof(lenBuf), 0);
     if (recvRes < 0) {
       L<<Logger::Error<<msgPrefix<<"Could not receive data (length) from master at "<<remote.toStringWithPort()<<", error:"<<stringerror()<<endl;
-      closesocket(sock);
+      try {
+        closesocket(sock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing master forwarding socket after recv() failed: "<<e.reason<<endl;
+      }
       continue;
     }
     int packetLen = lenBuf[0]*256+lenBuf[1];
@@ -646,10 +671,20 @@ int PacketHandler::forwardPacket(const string &msgPrefix, DNSPacket *p, DomainIn
     recvRes = recv(sock, &buf, packetLen, 0);
     if (recvRes < 0) {
       L<<Logger::Error<<msgPrefix<<"Could not receive data (dnspacket) from master at "<<remote.toStringWithPort()<<", error:"<<stringerror()<<endl;
-      closesocket(sock);
+      try {
+        closesocket(sock);
+      }
+      catch(const PDNSException& e) {
+        L<<Logger::Error<<"Error closing master forwarding socket after recv() failed: "<<e.reason<<endl;
+      }
       continue;
     }
-    closesocket(sock);
+    try {
+      closesocket(sock);
+    }
+    catch(const PDNSException& e) {
+      L<<Logger::Error<<"Error closing master forwarding socket: "<<e.reason<<endl;
+    }
 
     try {
       MOADNSParser mdp(buf, recvRes);
diff --git a/pdns/sstuff.hh b/pdns/sstuff.hh
index c2ccc2d181..a730a8787a 100644
--- a/pdns/sstuff.hh
+++ b/pdns/sstuff.hh
@@ -78,7 +78,12 @@ public:
 
   ~Socket()
   {
-    closesocket(d_socket);
+    try {
+      closesocket(d_socket);
+    }
+    catch(const PDNSException& e) {
+    }
+
     delete[] d_buffer;
   }
 
diff --git a/pdns/tcpreceiver.cc b/pdns/tcpreceiver.cc
index 995cdb465f..1aa6cc647c 100644
--- a/pdns/tcpreceiver.cc
+++ b/pdns/tcpreceiver.cc
@@ -260,7 +260,12 @@ void *TCPNameserver::doConnection(void *data)
   if(getpeername(fd, (struct sockaddr *)&remote, &remotelen) < 0) {
     L<<Logger::Warning<<"Received question from socket which had no remote address, dropping ("<<stringerror()<<")"<<endl;
     d_connectionroom_sem->post();
-    closesocket(fd);
+    try {
+      closesocket(fd);
+    }
+    catch(const PDNSException& e) {
+      L<<Logger::Error<<"Error closing TCP socket: "<<e.reason<<endl;
+    }
     return 0;
   }
 
@@ -387,7 +392,13 @@ void *TCPNameserver::doConnection(void *data)
     L << Logger::Error << "TCP Connection Thread caught unknown exception." << endl;
   }
   d_connectionroom_sem->post();
-  closesocket(fd);
+
+  try {
+    closesocket(fd);
+  }
+  catch(const PDNSException& e) {
+    L<<Logger::Error<<"Error closing TCP socket: "<<e.reason<<endl;
+  }
 
   return 0;
 }
diff --git a/pdns/utility.hh b/pdns/utility.hh
index 8ea95fadc1..d197c7c743 100644
--- a/pdns/utility.hh
+++ b/pdns/utility.hh
@@ -92,9 +92,6 @@ public:
   typedef int sock_t;
   typedef ::socklen_t socklen_t;
 
-  //! Closes a socket.
-  static int closesocket( sock_t socket );
-
   //! Connect with timeout
   // Returns:
   //    > 0 on success
-- 
2.47.2