fix TCP outgoing error handling, fixing possible crashes under high load

improve TCP error message reporting, differentiating errors, timeouts

git-svn-id: svn://svn.powerdns.com/pdns/trunk/pdns@706 d19b8d6e-7fed-0310-83ef-9ca221ded41b

diff --git a/pdns/lwres.cc b/pdns/lwres.cc
index cf50b063313b2b101004cab0d587cebd60670493..410fa3f37845b3894b7f4266b337cd664bcebd02 100644 (file)
--- a/pdns/lwres.cc
+++ b/pdns/lwres.cc
@@ -102,21 +102,22 @@ int LWRes::asyncresolve(uint32_t ip, const string& domain, int type, bool doTCP,
     const char *msgP=(const char*)&*vpacket.begin();
     string packet=string(lenP, lenP+2)+string(msgP, msgP+vpacket.size());
 
-    if(asendtcp(packet, &s) == 0) {
-      return 0;
-    }
-    
+    ret=asendtcp(packet, &s);
+    if(!(ret>0))           
+      return ret;
+
     packet.clear();
-    if(arecvtcp(packet,2, &s)==0) {
-      return 0;
-    }
+    ret=arecvtcp(packet, 2, &s);
+    if(!(ret > 0))
+      return ret;
 
     memcpy(&len, packet.c_str(), 2);
     len=ntohs(len);
 
-    if(arecvtcp(packet, len, &s)==0) {
-      return 0;
-    }
+    ret=arecvtcp(packet, len, &s);
+    if(!(ret > 0))
+      return ret;
+    
     if(len > (unsigned int)d_bufsize) {
       d_bufsize=len;
       delete[] d_buf;

diff --git a/pdns/mtasker.cc b/pdns/mtasker.cc
index 80fd3bab770c2be7cdd491c36c1060897fde2056..67ecfa364c18e4aa512cd6e60ce1da36b453f4d6 100644 (file)
--- a/pdns/mtasker.cc
+++ b/pdns/mtasker.cc
@@ -201,6 +201,8 @@ template<class Key, class Val>void MTasker<Key,Val>::yield()
     \param key Key of the event for which threads may be waiting
     \param val If non-zero, pointer to the content of the event
     \return Returns -1 in case of error, 0 if there were no waiters, 1 if a thread was woken up.
+
+    WARNING: when passing val as zero, d_waitval is undefined, and hence waitEvent will return undefined!
 */
 template<class EventKey, class EventVal>int MTasker<EventKey,EventVal>::sendEvent(const EventKey& key, const EventVal* val)
 {

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 70e15d2a774bf2999c825ee41c73b0b63fd2c90d..10b78da75c44834b4534cce1f61a2db1b90eeded 100644 (file)
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -55,7 +55,6 @@
 StatBag S;
 #endif
 
-
 using namespace boost;
 
 #ifdef __FreeBSD__           // see cvstrac ticket #26
@@ -130,36 +129,48 @@ static map<int,PacketID> d_tcpclientreadsocks, d_tcpclientwritesocks;
 typedef MTasker<PacketID,string> MT_t;
 MT_t* MT;
 
+// -1 is error, 0 is timeout, 1 is success
 int asendtcp(const string& data, Socket* sock) 
 {
   PacketID pident;
   pident.sock=sock;
   pident.outMSG=data;
-  string packet;
 
   d_tcpclientwritesocks[sock->getHandle()]=pident;
+  string packet;
 
   int ret=MT->waitEvent(pident,&packet,1);
+
   if(!ret || ret==-1) { // timeout
     d_tcpclientwritesocks.erase(sock->getHandle());
   }
+  else if(packet.size() !=data.size()) { // main loop tells us what it sent out, or empty in case of an error
+    return -1;
+  }
+  
   return ret;
 }
 
 // -1 is error, 0 is timeout, 1 is success
 int arecvtcp(string& data, int len, Socket* sock) 
 {
-  data="";
+  //  cerr<<"arecvtcp called for "<<len<<" bytes\n";
+  data.clear();
   PacketID pident;
   pident.sock=sock;
   pident.inNeeded=len;
-
+  //  cerr<<"Adding fd to clientreadsocks: "<<sock->getHandle()<<endl;
   d_tcpclientreadsocks[sock->getHandle()]=pident;
 
   int ret=MT->waitEvent(pident,&data,1);
+  //  cerr<<"ret in arecvtcp: "<<ret<<", data.size(): "<<data.size()<<"\n";
   if(!ret || ret==-1) { // timeout
     d_tcpclientreadsocks.erase(sock->getHandle());
   }
+  else if(data.empty()) {// error, EOF or other
+    return -1;
+  }
+
   return ret;
 }
 
@@ -1186,8 +1197,7 @@ int main(int argc, char **argv)
        if(FD_ISSET(i->first, &readfds)) { // can we receive
          shared_array<char> buffer(new char[i->second.inNeeded]);
 
-         int ret=read(i->first, buffer.get(), min(i->second.inNeeded,200));
-         // cerr<<"Read returned "<<ret<<endl;
+         int ret=read(i->first, buffer.get(), i->second.inNeeded);
          if(ret > 0) {
            i->second.inMSG.append(&buffer[0], &buffer[ret]);
            i->second.inNeeded-=ret;
@@ -1198,15 +1208,18 @@ int main(int argc, char **argv)
              
              d_tcpclientreadsocks.erase((i++));
              haveErased=true;
-             MT->sendEvent(pid, &msg);   // XXX DODGY
+             MT->sendEvent(pid, &msg);   // XXX DODGY (why? msg is copied nicely by sendEvent)
            }
            else {
              // cerr<<"Still have "<<i->second.inNeeded<<" left to go"<<endl;
            }
          }
          else {
-           //      cerr<<"when reading ret="<<ret<<endl;
-           // XXX FIXME I think some stuff needs to happen here - like send an EOF event
+           PacketID pid=i->second;
+           d_tcpclientreadsocks.erase((i++));
+           haveErased=true;
+           string empty;
+           MT->sendEvent(pid, &empty); // this conveys error status
          }
        }
        if(!haveErased)
@@ -1217,22 +1230,22 @@ int main(int argc, char **argv)
       for(map<int,PacketID>::iterator i=d_tcpclientwritesocks.begin(); i!=d_tcpclientwritesocks.end(); ) { 
        bool haveErased=false;
        if(FD_ISSET(i->first, &writefds)) { // can we send over TCP
-         // cerr<<"Socket "<<i->first<<" available for writing"<<endl;
          int ret=write(i->first, i->second.outMSG.c_str(), i->second.outMSG.size() - i->second.outPos);
          if(ret > 0) {
            i->second.outPos+=ret;
            if(i->second.outPos==i->second.outMSG.size()) {
-             // cerr<<"Sent out entire load of "<<i->second.outMSG.size()<<" bytes"<<endl;
              PacketID pid=i->second;
              d_tcpclientwritesocks.erase(i++);   // erase!
              haveErased=true;
-             MT->sendEvent(pid, 0);
+             MT->sendEvent(pid, &pid.outMSG);  // send back what we sent to convey everything is ok
            }
-
          }
-         else { 
-           //      cerr<<"ret="<<ret<<" when writing"<<endl;
-           // XXX FIXME I think some stuff needs to happen here - like send an EOF event
+         else {  // error or EOF
+           PacketID pid=i->second;             
+           d_tcpclientwritesocks.erase(i++);  
+           haveErased=true;
+           string sent;
+           MT->sendEvent(pid, &sent);         // we convey error status by sending empty string
          }
        }
        if(!haveErased)

diff --git a/pdns/syncres.cc b/pdns/syncres.cc
index 44fce76725c9f07c0ca64ca408f8e20dd8039c02..e2d87d0f92bfa74ba4db2647dcd9741ae52e7e81 100644 (file)
--- a/pdns/syncres.cc
+++ b/pdns/syncres.cc
@@ -453,12 +453,12 @@ int SyncRes::doResolveAt(set<string, CIStringCompare> nameservers, string auth,
          int ret=d_lwr.asyncresolve(*remoteIP, qname, qtype.getCode(), doTCP, &d_now);    // <- we go out on the wire!
          if(ret != 1) {
            if(ret==0) {
-             LOG<<prefix<<qname<<": timeout resolving"<<endl;
+             LOG<<prefix<<qname<<": timeout resolving "<< (doTCP ? "over TCP" : "")<<endl;
              d_timeouts++;
              s_outgoingtimeouts++;
            }
            else
-             LOG<<prefix<<qname<<": error resolving"<<endl;
+             LOG<<prefix<<qname<<": error resolving "<< (doTCP ? "over TCP" : "") << endl;
            
            s_nsSpeeds[*tns].submit(1000000, &d_now); // 1 sec