Changeset 678

Timestamp:

04/11/06 18:09:50 (7 years ago)

Author:

ahu

Message:

add counts for broken packets
add switch to prevent logging rather common errors
removed duplicate answer parsing (!!)
improved TCP timeout code, previously we could time out a running TCP query, which was wrong
improve error message consistency slightly

Location:

trunk/pdns/pdns

Files:

• dnsparser.cc (modified) (1 diff)
• pdns_recursor.cc (modified) (20 diffs)
• rec_channel_rec.cc (modified) (1 diff)
• syncres.hh (modified) (1 diff)

trunk/pdns/pdns/dnsparser.cc

@@ -236 +236 @@
   }
   catch(out_of_range &re) {
-    throw MOADNSException("Packet parsing error, out of bounds: "+string(re.what()));
+    throw MOADNSException("Error parsing packet of "+lexical_cast<string>(len)+" bytes, out of bounds: "+string(re.what()));
   }
 }

trunk/pdns/pdns/pdns_recursor.cc

@@ -269 +269 @@
 }
 
+map<uint32_t, uint32_t> g_tcpClientCounts;
+
+struct TCPConnection
+{
+  int fd;
+  enum {BYTE0, BYTE1, GETQUESTION, DONE} state;
+  int qlen;
+  int bytesread;
+  struct sockaddr_in remote;
+  char data[65535];
+  time_t startTime;
+
+  void closeAndCleanup()
+  {
+    close(fd);
+    if(!g_tcpClientCounts[remote.sin_addr.s_addr]--) 
+      g_tcpClientCounts.erase(remote.sin_addr.s_addr);
+  }
+};
+
+vector<TCPConnection> g_tcpconnections; // all *running* TCP/IP questions (from clients)
+
 void startDoResolve(void *p)
 {
@@ -352 +374 @@
 
       int ret=writev(dc->d_socket, iov, 2);
-
-      if(ret <= 0 ) 
-        L<<Logger::Error<<"Error writing TCP answer to "<<dc->getRemote()<<": "<< (ret ? strerror(errno) : "EOF") <<endl;
+      bool hadError=true;
+
+      if(ret == 0) 
+        L<<Logger::Error<<"EOF writing TCP answer to "<<dc->getRemote()<<endl;
+      else if(ret < 0 )  
+        L<<Logger::Error<<"Error writing TCP answer to "<<dc->getRemote()<<": "<< strerror(errno) <<endl;
       else if((unsigned int)ret != 2 + packet.size())
         L<<Logger::Error<<"Oops, partial answer sent to "<<dc->getRemote()<<" for "<<dc->d_mdp.d_qname<<" (size="<< (2 + packet.size()) <<", sent "<<ret<<")"<<endl;
+      else
+        hadError=false;
+
+      for(vector<TCPConnection>::iterator i=g_tcpconnections.begin();i!=g_tcpconnections.end();++i) {
+        if(i->fd == dc->d_socket) {
+          if(hadError) {
+            i->closeAndCleanup();
+            g_tcpconnections.erase(i);
+          }
+          else {
+            i->state=TCPConnection::BYTE0;
+            i->startTime=time(0); // needs to be current, TCP is slow anyhow
+          }
+          break;
+        }
+      }
     }
 
@@ -618 +659 @@
 }
 
-map<uint32_t, uint32_t> g_tcpClientCounts;
-struct TCPConnection
-{
-  int fd;
-  enum {BYTE0, BYTE1, GETQUESTION} state;
-  int qlen;
-  int bytesread;
-  struct sockaddr_in remote;
-  char data[65535];
-  time_t startTime;
-
-  void closeAndCleanup()
-  {
-    close(fd);
-    if(!g_tcpClientCounts[remote.sin_addr.s_addr]--) 
-      g_tcpClientCounts.erase(remote.sin_addr.s_addr);
-  }
-};
+
 
 #if 0
@@ -702 +726 @@
     ::arg().set("trace","if we should output heaps of logging")="off";
     ::arg().set("daemon","Operate as a daemon")="yes";
+    ::arg().set("log-common-errors","If we should log rather common errors")="yes";
     ::arg().set("chroot","switch to chroot jail")="";
     ::arg().set("setgid","If set, change group id to this gid for more security")="";
@@ -755 +780 @@
 
     L<<Logger::Warning<<"Operating in "<<(sizeof(unsigned long)*8) <<" bits mode"<<endl;
-  L<<Logger::Warning<<"PowerDNS comes with ABSOLUTELY NO WARRANTY. "
-    "This is free software, and you are welcome to redistribute it "
-    "according to the terms of the GPL version 2."<<endl;
-
-
-  g_quiet=::arg().mustDo("quiet");
-  if(::arg().mustDo("trace")) {
+    L<<Logger::Warning<<"PowerDNS comes with ABSOLUTELY NO WARRANTY. "
+      "This is free software, and you are welcome to redistribute it "
+      "according to the terms of the GPL version 2."<<endl;
+    
+    
+    g_quiet=::arg().mustDo("quiet");
+    if(::arg().mustDo("trace")) {
       SyncRes::setLog(true);
       ::arg().set("quiet")="no";
       g_quiet=false;
-  }
+    }
+
+    bool logCommonErrors=::arg().mustDo("log-common-errors");
 
     makeUDPServerSockets();
@@ -815 +842 @@
     Utility::dropPrivs(newuid, newgid);
 
-    vector<TCPConnection> tcpconnections;
+
     counter=0;
     struct timeval now;
@@ -846 +873 @@
       int fdmax=max(d_clientsock, s_rcc.d_fd);
 
-      if(!tcpconnections.empty())
+      if(!g_tcpconnections.empty())
         gettimeofday(&now, 0);
 
       vector<TCPConnection> sweeped;
 
-      for(vector<TCPConnection>::iterator i=tcpconnections.begin();i!=tcpconnections.end();++i) {
-        if(now.tv_sec < i->startTime + tcpTimeout) {
-          FD_SET(i->fd, &readfds);
-          fdmax=max(fdmax,i->fd);
+      for(vector<TCPConnection>::iterator i=g_tcpconnections.begin();i!=g_tcpconnections.end();++i) {
+        if(i->state==TCPConnection::DONE || now.tv_sec < i->startTime + tcpTimeout) {  // don't timeout when we are working on the question!
           sweeped.push_back(*i);
+          if(i->state!=TCPConnection::DONE) { // we don't listen for data when we are processing the question
+            FD_SET(i->fd, &readfds);
+            fdmax=max(fdmax,i->fd);
+          }
         }
         else {
-          L<<Logger::Error<<"TCP timeout from client "<<inet_ntoa(i->remote.sin_addr)<<endl;
+          if(logCommonErrors)
+            L<<Logger::Error<<"TCP timeout from client "<<inet_ntoa(i->remote.sin_addr)<<endl;
           i->closeAndCleanup();
         }
       }
-      sweeped.swap(tcpconnections);
+      sweeped.swap(g_tcpconnections);
 
       for(vector<int>::const_iterator i=d_udpserversocks.begin(); i!=d_udpserversocks.end(); ++i) {
@@ -868 +898 @@
         fdmax=max(fdmax,*i);
       }
-      if(tcpconnections.size() < maxTcpClients) 
+      if(g_tcpconnections.size() < maxTcpClients) 
         for(tcpserversocks_t::const_iterator i=s_tcpserversocks.begin(); i!=s_tcpserversocks.end(); ++i) {
           FD_SET(*i, &readfds );
@@ -904 +934 @@
       }
 
-      if(FD_ISSET(d_clientsock,&readfds)) { // do we have a UDP question response?
+      if(FD_ISSET(d_clientsock,&readfds)) { // do we have a UDP question response from a server ("we are the client", hence d_clientsock)
         while((d_len=recvfrom(d_clientsock, data, sizeof(data), 0, (sockaddr *)&fromaddr, &addrlen)) >= 0) {
-        try {
-            DNSComboWriter dc(data, d_len, now);
-            dc.setRemote((struct sockaddr *)&fromaddr, addrlen);
-
-            if(dc.d_mdp.d_header.qr) {
+          dnsheader dh;
+          if(d_len >= sizeof(dnsheader)) {
+            memcpy(&dh, data, sizeof(dh));
+            
+            if(dh.qr) {
               pident.remote=fromaddr;
-              pident.id=dc.d_mdp.d_header.id;
+              pident.id=dh.id;
               string packet;
               packet.assign(data, d_len);
@@ -918 +948 @@
             }
             else 
-              L<<Logger::Warning<<"Ignoring question on outgoing socket from "<<dc.getRemote()<<endl;
-          }
-          catch(MOADNSException& mde) {
-            L<<Logger::Error<<"Unable to parse packet from remote UDP server "<< sockAddrToString((struct sockaddr_in*) &fromaddr, addrlen) <<": "<<mde.what()<<endl;
+              L<<Logger::Warning<<"Ignoring question on outgoing socket from "<< sockAddrToString((struct sockaddr_in*) &fromaddr, addrlen)  <<endl;
+          }
+          else {
+            g_stats.serverParseError++; 
+            if(logCommonErrors)
+              L<<Logger::Error<<"Unable to parse packet from remote UDP server "<< sockAddrToString((struct sockaddr_in*) &fromaddr, addrlen) <<": packet too small"<<endl;
           }
         }
@@ -929 +961 @@
         if(FD_ISSET(*i,&readfds)) { // do we have a new question on udp?
           while((d_len=recvfrom(*i, data, sizeof(data), 0, (sockaddr *)&fromaddr, &addrlen)) >= 0) {
-            g_stats.queryrate.pulse(now);
+            //      g_stats.queryrate.pulse(now);  // (broken)
             if(g_allowFrom && !g_allowFrom->match(&fromaddr)) {
               g_stats.unauthorizedUDP++;
@@ -940 +972 @@
               dc->setRemote((struct sockaddr *)&fromaddr, addrlen);
 
-              if(dc->d_mdp.d_header.qr)
-                L<<Logger::Error<<"Ignoring answer on server socket!"<<endl;
+              if(dc->d_mdp.d_header.qr) {
+                if(logCommonErrors)
+                  L<<Logger::Error<<"Ignoring answer from "<<dc->getRemote()<<" on server socket!"<<endl;
+              }
               else {
                 ++g_stats.qcounter;
@@ -950 +984 @@
             }
             catch(MOADNSException& mde) {
+              g_stats.clientParseError++; 
               L<<Logger::Error<<"Unable to parse packet from remote UDP client "<< sockAddrToString((struct sockaddr_in*) &fromaddr, addrlen) <<": "<<mde.what()<<endl;
             }
@@ -957 +992 @@
 
       for(tcpserversocks_t::const_iterator i=s_tcpserversocks.begin(); i!=s_tcpserversocks.end(); ++i) { 
-        if(FD_ISSET(*i ,&readfds)) { // do we have a new TCP connection?
+        if(FD_ISSET(*i ,&readfds)) { // do we have a new TCP connection from a client?
           struct sockaddr_in addr;
           socklen_t addrlen=sizeof(addr);
@@ -980 +1015 @@
             tc.remote=addr;
             tc.startTime=now.tv_sec;
-            tcpconnections.push_back(tc);
+            g_tcpconnections.push_back(tc);
           }
         }
@@ -1045 +1080 @@
       
       // very braindead TCP incoming question parser
-      for(vector<TCPConnection>::iterator i=tcpconnections.begin();i!=tcpconnections.end();++i) {
+      for(vector<TCPConnection>::iterator i=g_tcpconnections.begin();i!=g_tcpconnections.end();++i) {
         if(FD_ISSET(i->fd, &readfds)) {
           if(i->state==TCPConnection::BYTE0) {
@@ -1058 +1093 @@
             if(!bytes || bytes < 0) {
               i->closeAndCleanup();
-              tcpconnections.erase(i);
+              g_tcpconnections.erase(i);
               break;
             }
@@ -1070 +1105 @@
             }
             if(!bytes || bytes < 0) {
-              L<<Logger::Error<<"TCP Remote "<<sockAddrToString(&i->remote,sizeof(i->remote))<<" disconnected after first byte"<<endl;
+              if(logCommonErrors)
+                L<<Logger::Error<<"TCP client "<<sockAddrToString(&i->remote,sizeof(i->remote))<<" disconnected after first byte"<<endl;
               i->closeAndCleanup();
-              tcpconnections.erase(i);
+              g_tcpconnections.erase(i);
               break;
             }
-            
           }
           else if(i->state==TCPConnection::GETQUESTION) {
             int bytes=read(i->fd,i->data + i->bytesread,i->qlen - i->bytesread);
             if(!bytes || bytes < 0) {
-              L<<Logger::Error<<"TCP Remote "<<sockAddrToString(&i->remote,sizeof(i->remote))<<" disconnected while reading question body"<<endl;
+              L<<Logger::Error<<"TCP client "<<sockAddrToString(&i->remote,sizeof(i->remote))<<" disconnected while reading question body"<<endl;
               i->closeAndCleanup();
-              tcpconnections.erase(i);
+              g_tcpconnections.erase(i);
               break;
             }
             i->bytesread+=bytes;
             if(i->bytesread==i->qlen) {
-              i->state=TCPConnection::BYTE0;
+              i->state=TCPConnection::DONE; // this makes us immune from timeouts, from now on *we* are responsible
               DNSComboWriter* dc=0;
               try {
@@ -1093 +1128 @@
               }
               catch(MOADNSException &mde) {
-                L<<Logger::Error<<"Unable to parse packet from remote TCP client "<<sockAddrToString(&i->remote,sizeof(i->remote))<<endl;
+                g_stats.clientParseError++; 
+                L<<Logger::Error<<"Unable to parse packet from TCP client "<<sockAddrToString(&i->remote,sizeof(i->remote))<<endl;
                 i->closeAndCleanup();
-                tcpconnections.erase(i);
+                g_tcpconnections.erase(i);
                 break;
               }
@@ -1108 +1144 @@
                 ++g_stats.tcpqcounter;
                 MT->makeThread(startDoResolve, dc);
+                break;
               }
             }

trunk/pdns/pdns/rec_channel_rec.cc

@@ -144 +144 @@
   addGetStat("tcp-client-overflow", &g_stats.tcpClientOverflow);
 
+  addGetStat("client-parse-errors", &g_stats.clientParseError);
+  addGetStat("server-parse-errors", &g_stats.serverParseError);
+
   addGetStat("answers0-1", &g_stats.answers0_1);
   addGetStat("answers1-10", &g_stats.answers1_10);

trunk/pdns/pdns/syncres.hh

@@ -357 +357 @@
   uint64_t unauthorizedTCP;
   uint64_t tcpClientOverflow;
+  uint64_t clientParseError;
+  uint64_t serverParseError;
 };