From fd378ba9299a056923efd7c80de6cd2f3010f4f9 Mon Sep 17 00:00:00 2001
From: Pieter Lexis <pieter.lexis@powerdns.com>
Date: Mon, 23 Nov 2015 15:21:07 +0100
Subject: [PATCH] pipe: don't crash on exceptions

This commit cleans up the PipeBackend code so it handles all exceptions
itself. Be it initialization errors (i.e. "file not found", "command not
executable"), or error during runtime ("data not returned in pipe
format"). When these errors occur, we now recycle the coprocess so the
rest of the nameserver keeps running. This behaviour is similar to e.g.
an unavailable database server.

Also, make the error-message for ArgMap::asNum display the value of the
setting.

Closes #2619
---
 modules/pipebackend/pipebackend.cc | 289 ++++++++++++++++-------------
 modules/pipebackend/pipebackend.hh |   4 +-
 pdns/arguments.cc                  |   2 +-
 3 files changed, 168 insertions(+), 127 deletions(-)

diff --git a/modules/pipebackend/pipebackend.cc b/modules/pipebackend/pipebackend.cc
index 0fc29ae4e..41e19df4c 100644
--- a/modules/pipebackend/pipebackend.cc
+++ b/modules/pipebackend/pipebackend.cc
@@ -51,13 +51,17 @@ void CoWrapper::launch()
    if(d_cp)
       return;
 
+   if(d_command.empty())
+     throw ArgException("pipe-command is not specified");
+
    if(isUnixSocket(d_command))
      d_cp = new UnixRemote(d_command, d_timeout);
    else
-     d_cp = new CoProcess(d_command, d_timeout); 
+     d_cp = new CoProcess(d_command, d_timeout);
+
    d_cp->send("HELO\t"+std::to_string(d_abiVersion));
    string banner;
-   d_cp->receive(banner); 
+   d_cp->receive(banner);
    L<<Logger::Error<<"Backend launched with banner: "<<banner<<endl;
 }
 
@@ -95,82 +99,108 @@ PipeBackend::PipeBackend(const string &suffix)
    signal(SIGCHLD, SIG_IGN);
    setArgPrefix("pipe"+suffix);
    try {
-     d_coproc=shared_ptr<CoWrapper>(new CoWrapper(getArg("command"), getArgAsNum("timeout"), getArgAsNum("abi-version")));
-     d_regex=getArg("regex").empty() ? 0 : new Regex(getArg("regex"));
-     d_regexstr=getArg("regex");
-     d_abiVersion = getArgAsNum("abi-version");
+     launch();
    }
    catch(const ArgException &A) {
-      L<<Logger::Error<<kBackendId<<" Fatal argument error: "<<A.reason<<endl;
-      throw;
+      L<<Logger::Error<<kBackendId<<" Unable to launch, fatal argument error: "<<A.reason<<endl;
    }
    catch(...) {
       throw;
    }
 }
 
+void PipeBackend::launch()
+{
+  if(d_coproc)
+    return;
+
+  try {
+    d_regex=getArg("regex").empty() ? 0 : new Regex(getArg("regex"));
+    d_regexstr=getArg("regex");
+    d_abiVersion = getArgAsNum("abi-version");
+    d_coproc=unique_ptr<CoWrapper> (new CoWrapper(getArg("command"), getArgAsNum("timeout"), getArgAsNum("abi-version")));
+  }
+
+  catch(const ArgException &A) {
+    cleanup();
+    throw;
+  }
+}
+
+/*
+ * Cleans up the co-process wrapper
+ */
+void PipeBackend::cleanup()
+{
+  d_coproc.reset(0);
+  delete d_regex;
+  d_regexstr = string();
+  d_abiVersion = 0;
+}
+
 void PipeBackend::lookup(const QType& qtype,const DNSName& qname, DNSPacket *pkt_p,  int zoneId)
 {
-   try {
-      d_disavow=false;
-      if(d_regex && !d_regex->match(qname.toStringNoDot())) {
-         if(::arg().mustDo("query-logging"))
-            L<<Logger::Error<<"Query for '"<<qname<<"' failed regex '"<<d_regexstr<<"'"<<endl;
-         d_disavow=true; // don't pass to backend
-      } else {
-         ostringstream query;
-         string localIP="0.0.0.0";
-         string remoteIP="0.0.0.0";
-         Netmask realRemote("0.0.0.0/0");
-         if (pkt_p) {
-            localIP=pkt_p->getLocal();
-            realRemote = pkt_p->getRealRemote();
-            remoteIP = pkt_p->getRemote();
-         }
-         // abi-version = 1
-         // type    qname           qclass  qtype   id      remote-ip-address
-         query<<"Q\t"<<qname.toStringNoDot()<<"\tIN\t"<<qtype.getName()<<"\t"<<zoneId<<"\t"<<remoteIP;
-
-         // add the local-ip-address if abi-version is set to 2
-         if (d_abiVersion >= 2)
-            query<<"\t"<<localIP;
-         if(d_abiVersion >= 3)
-           query <<"\t"<<realRemote.toString(); 
-
-         if(::arg().mustDo("query-logging"))
-            L<<Logger::Error<<"Query: '"<<query.str()<<"'"<<endl;
-         d_coproc->send(query.str());
+  try {
+    launch();
+    d_disavow=false;
+    if(d_regex && !d_regex->match(qname.toStringNoDot())) {
+      if(::arg().mustDo("query-logging"))
+        L<<Logger::Error<<"Query for '"<<qname<<"' failed regex '"<<d_regexstr<<"'"<<endl;
+      d_disavow=true; // don't pass to backend
+    } else {
+      ostringstream query;
+      string localIP="0.0.0.0";
+      string remoteIP="0.0.0.0";
+      Netmask realRemote("0.0.0.0/0");
+      if (pkt_p) {
+        localIP=pkt_p->getLocal();
+        realRemote = pkt_p->getRealRemote();
+        remoteIP = pkt_p->getRemote();
       }
-   }
-   catch(PDNSException &ae) {
-      L<<Logger::Error<<kBackendId<<" Error from coprocess: "<<ae.reason<<endl;
-      throw; // hop
-   }
-   d_qtype=qtype;
-   d_qname=qname;
+      // abi-version = 1
+      // type    qname           qclass  qtype   id      remote-ip-address
+      query<<"Q\t"<<qname.toStringNoDot()<<"\tIN\t"<<qtype.getName()<<"\t"<<zoneId<<"\t"<<remoteIP;
+
+      // add the local-ip-address if abi-version is set to 2
+      if (d_abiVersion >= 2)
+        query<<"\t"<<localIP;
+      if(d_abiVersion >= 3)
+        query <<"\t"<<realRemote.toString(); 
+
+      if(::arg().mustDo("query-logging"))
+        L<<Logger::Error<<"Query: '"<<query.str()<<"'"<<endl;
+      d_coproc->send(query.str());
+    }
+  }
+  catch(PDNSException &pe) {
+    L<<Logger::Error<<kBackendId<<" Error from coprocess: "<<pe.reason<<endl;
+    d_disavow = true;
+  }
+  d_qtype=qtype;
+  d_qname=qname;
 }
 
 bool PipeBackend::list(const DNSName& target, int inZoneId, bool include_disabled)
 {
-   try {
-      d_disavow=false;
-      ostringstream query;
-// The question format:
-
-// type    qname           qclass  qtype   id      ip-address
-      if (d_abiVersion >= 4)
-        query<<"AXFR\t"<<inZoneId<<"\t"<<target.toStringNoDot();
-      else
-        query<<"AXFR\t"<<inZoneId;
-
-      d_coproc->send(query.str());
-   }
-   catch(PDNSException &ae) {
-      L<<Logger::Error<<kBackendId<<" Error from coprocess: "<<ae.reason<<endl;
-      throw;
-   }
-   d_qname=DNSName(itoa(inZoneId)); // why do we store a number here??
-   return true;
+  try {
+    launch();
+    d_disavow=false;
+    ostringstream query;
+    // The question format:
+
+    // type    qname           qclass  qtype   id      ip-address
+    if (d_abiVersion >= 4)
+      query<<"AXFR\t"<<inZoneId<<"\t"<<target.toStringNoDot();
+    else
+      query<<"AXFR\t"<<inZoneId;
+
+    d_coproc->send(query.str());
+  }
+  catch(PDNSException &ae) {
+    L<<Logger::Error<<kBackendId<<" Error from coprocess: "<<ae.reason<<endl;
+  }
+  d_qname=DNSName(itoa(inZoneId)); // why do we store a number here??
+  return true;
 }
 
 string PipeBackend::directBackendCmd(const string &query) {
@@ -180,24 +210,25 @@ string PipeBackend::directBackendCmd(const string &query) {
   ostringstream oss;
 
   try {
-      ostringstream oss;
-      oss<<"CMD\t"<<query;
-      d_coproc->send(oss.str());
-   }
-   catch(PDNSException &ae) {
-      L<<Logger::Error<<kBackendId<<" Error from coprocess: "<<ae.reason<<endl;
-      throw;
-   }
-   oss.str("");
-
-   while(true) {
-     string line;
-     d_coproc->receive(line);
-     if (line == "END") break;
-     oss << line << std::endl;
-   };
-
-   return oss.str();
+    launch();
+    ostringstream oss;
+    oss<<"CMD\t"<<query;
+    d_coproc->send(oss.str());
+  }
+  catch(PDNSException &ae) {
+    L<<Logger::Error<<kBackendId<<" Error from coprocess: "<<ae.reason<<endl;
+    cleanup();
+  }
+  oss.str("");
+
+  while(true) {
+    string line;
+    d_coproc->receive(line);
+    if (line == "END") break;
+    oss << line << std::endl;
+  };
+
+  return oss.str();
 }
 
 //! For the dynamic loader
@@ -214,47 +245,49 @@ DNSBackend *PipeBackend::maker()
 
 PipeBackend::~PipeBackend()
 {
-   delete d_regex;
+  cleanup();
 }
 
 bool PipeBackend::get(DNSResourceRecord &r)
 {
-   if(d_disavow) // this query has been blocked
-      return false;
-
-   string line;
-
-   // The answer format:
-   // DATA    qname           qclass  qtype   ttl     id      content 
-   unsigned int extraFields = 0;
-   if(d_abiVersion >= 3)
-     extraFields = 2;
-     
-   for(;;) {
+  if(d_disavow) // this query has been blocked
+    return false;
+
+  string line;
+
+  // The answer format:
+  // DATA    qname           qclass  qtype   ttl     id      content 
+  unsigned int extraFields = 0;
+  if(d_abiVersion >= 3)
+    extraFields = 2;
+
+  try{
+    launch();
+    for(;;) {
       d_coproc->receive(line);
       vector<string>parts;
       stringtok(parts,line,"\t");
       if(parts.empty()) {
-         L<<Logger::Error<<kBackendId<<" Coprocess returned empty line in query for "<<d_qname<<endl;
-         throw PDNSException("Format error communicating with coprocess");
+        L<<Logger::Error<<kBackendId<<" Coprocess returned empty line in query for "<<d_qname<<endl;
+        throw PDNSException("Format error communicating with coprocess");
       }
       else if(parts[0]=="FAIL") {
-         throw DBException("coprocess returned a FAIL");
+        throw DBException("coprocess returned a FAIL");
       }
       else if(parts[0]=="END") {
-         return false;
+        return false;
       }
       else if(parts[0]=="LOG") {
-         L<<Logger::Error<<"Coprocess: "<<line.substr(4)<<endl;
-         continue;
+        L<<Logger::Error<<"Coprocess: "<<line.substr(4)<<endl;
+        continue;
       }
       else if(parts[0]=="DATA") { // yay
-         if(parts.size() < 7 + extraFields) {
-            L<<Logger::Error<<kBackendId<<" Coprocess returned incomplete or empty line in data section for query for "<<d_qname<<endl;
-            throw PDNSException("Format error communicating with coprocess in data section");
-            // now what?
-         }
-         
+        if(parts.size() < 7 + extraFields) {
+          L<<Logger::Error<<kBackendId<<" Coprocess returned incomplete or empty line in data section for query for "<<d_qname<<endl;
+          throw PDNSException("Format error communicating with coprocess in data section");
+          // now what?
+        }
+
          if(d_abiVersion >= 3) {
            r.scopeMask = std::stoi(parts[1]);
            r.auth = (parts[2] == "1");
@@ -266,29 +299,35 @@ bool PipeBackend::get(DNSResourceRecord &r)
          r.qtype=parts[3+extraFields];
          r.ttl=pdns_stou(parts[4+extraFields]);
          r.domain_id=std::stoi(parts[5+extraFields]);
-         
-         if(r.qtype.getCode() != QType::MX && r.qtype.getCode() != QType::SRV) {
-           r.content.clear();
-           for(unsigned int n= 6 + extraFields; n < parts.size(); ++n) {
-             if(n!=6+extraFields)
-               r.content.append(1,' ');
-             r.content.append(parts[n]);
-           }
-         }
-         else {
-           if(parts.size()< 8 + extraFields) {
+
+        if(r.qtype.getCode() != QType::MX && r.qtype.getCode() != QType::SRV) {
+          r.content.clear();
+          for(unsigned int n= 6 + extraFields; n < parts.size(); ++n) {
+            if(n!=6+extraFields)
+              r.content.append(1,' ');
+            r.content.append(parts[n]);
+          }
+        }
+        else {
+          if(parts.size()< 8 + extraFields) {
             L<<Logger::Error<<kBackendId<<" Coprocess returned incomplete MX/SRV line in data section for query for "<<d_qname<<endl;
             throw PDNSException("Format error communicating with coprocess in data section of MX/SRV record");
-           }
-           
-           r.content=parts[6+extraFields]+" "+parts[7+extraFields];
-         }
-         break;
+          }
+
+          r.content=parts[6+extraFields]+" "+parts[7+extraFields];
+        }
+        break;
       }
       else
-         throw PDNSException("Coprocess backend sent incorrect response '"+line+"'");
-   }   
-   return true;
+        throw PDNSException("Coprocess backend sent incorrect response '"+line+"'");
+    }
+  }
+  catch (PDNSException &pe) {
+    L<<Logger::Error<<kBackendId<<" "<<pe.reason<<endl;
+    cleanup();
+    return false;
+  }
+  return true;
 }
 
 //
diff --git a/modules/pipebackend/pipebackend.hh b/modules/pipebackend/pipebackend.hh
index f3053b8c8..48ac0898e 100644
--- a/modules/pipebackend/pipebackend.hh
+++ b/modules/pipebackend/pipebackend.hh
@@ -44,7 +44,9 @@ public:
   static DNSBackend *maker();
   
 private:
-  shared_ptr<CoWrapper> d_coproc;
+  void launch();
+  void cleanup();
+  unique_ptr<CoWrapper> d_coproc;
   DNSName d_qname;
   QType d_qtype;
   Regex* d_regex;
diff --git a/pdns/arguments.cc b/pdns/arguments.cc
index 8e9294b4e..cba7796d4 100644
--- a/pdns/arguments.cc
+++ b/pdns/arguments.cc
@@ -255,7 +255,7 @@ int ArgvMap::asNum(const string &arg, int def)
   cptr_orig = params[arg].c_str();
   retval = static_cast<int>(strtol(cptr_orig, &cptr_ret, 0));
   if (!retval && cptr_ret == cptr_orig)
-   throw ArgException("'"+arg+string("' is not valid number"));
+   throw ArgException("'"+arg+"' value '"+string(cptr_orig) + string( "' is not a valid number"));
 
   return retval;
 }
-- 
2.40.0