From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Wed, 10 Jul 2019 14:14:01 +0000 (+0200)
Subject: Clean up the CDB code to make it usable from dnsdist
X-Git-Tag: dnsdist-1.4.0-rc2~9^2~24
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b4aac52add81422c602da1c3f360fcb679275403;p=pdns

Clean up the CDB code to make it usable from dnsdist
---

diff --git a/modules/tinydnsbackend/cdb.cc b/modules/tinydnsbackend/cdb.cc
index d836d34ae..2fa1cd7b7 100644
--- a/modules/tinydnsbackend/cdb.cc
+++ b/modules/tinydnsbackend/cdb.cc
@@ -22,33 +22,30 @@
 #ifdef HAVE_CONFIG_H
 #include "config.h"
 #endif
-#include "cdb.hh"
-#include <cdb.h>
-#include "pdns/misc.hh"
-#include "pdns/iputils.hh"
-#include <utility>
 
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+#include "cdb.hh"
 
 CDB::CDB(const string &cdbfile)
 {
   d_fd = open(cdbfile.c_str(), O_RDONLY);
   if (d_fd < 0)
   {
-    g_log<<Logger::Error<<"Failed to open cdb database file '"<<cdbfile<<"'. Error: "<<stringerror()<<endl;
-    throw new PDNSException("Failed to open cdb database file '"+cdbfile+"'. Error: " + stringerror());
+    throw std::runtime_error("Failed to open cdb database file '"+cdbfile+"'. Error: " + stringerror());
   }
 
   memset(&d_cdbf,0,sizeof(struct cdb_find));
   int cdbinit = cdb_init(&d_cdb, d_fd);
   if (cdbinit < 0)
   {
-    g_log<<Logger::Error<<"Failed to initialize cdb structure. ErrorNr: '"<<cdbinit<<endl;
-    throw new PDNSException("Failed to initialize cdb structure.");
+    close(d_fd);
+    d_fd = -1;
+    throw std::runtime_error("Failed to initialize cdb structure. ErrorNt: '" + std::to_string(cdbinit) + "'");
   }
-
-  d_key = NULL;
-  d_seqPtr = 0;
-  d_searchType = SearchKey;
 }
 
 CDB::~CDB() {
@@ -61,18 +58,18 @@ int CDB::searchKey(const string &key) {
 
   // A 'bug' in tinycdb (the lib used for reading the CDB files) means we have to copy the key because the cdb_find struct
   // keeps a pointer to it.
-  d_key = strdup(key.c_str());
-  return cdb_findinit(&d_cdbf, &d_cdb, d_key, key.size());
+  d_key = key;
+  return cdb_findinit(&d_cdbf, &d_cdb, d_key.c_str(), d_key.size());
 }
 
 bool CDB::searchSuffix(const string &key) {
   d_searchType = SearchSuffix;
 
   //See CDB::searchKey()
-  d_key = strdup(key.c_str());
+  d_key = key;
 
   // We are ok with a search on things, but we do want to know if a record with that key exists.........
-  bool hasDomain = (cdb_find(&d_cdb, key.c_str(), key.size()) == 1);
+  bool hasDomain = (cdb_find(&d_cdb, d_key.c_str(), d_key.size()) == 1);
   if (hasDomain) {
     cdb_seqinit(&d_seqPtr, &d_cdb);
   }
@@ -103,33 +100,32 @@ bool CDB::readNext(pair<string, string> &value) {
     pos = cdb_keypos(&d_cdb);
     len = cdb_keylen(&d_cdb);
 
-    char *key = (char *)malloc(len);
-    cdb_read(&d_cdb, key, len, pos);
+    std::string key;
+    key.resize(len);
+    cdb_read(&d_cdb, &key[0], len, pos);
 
     if (d_searchType == SearchSuffix) {
-      char *p = strstr(key, d_key);
-      if (p == NULL) {
-        free(key);
+      char *p = strstr(const_cast<char*>(key.c_str()), d_key.c_str());
+      if (p == nullptr) {
         continue;
       }
     }
-    string skey(key, len);
-    free(key);
 
     pos = cdb_datapos(&d_cdb);
     len = cdb_datalen(&d_cdb);
-    char *val = (char *)malloc(len);
-    cdb_read(&d_cdb, val, len, pos);
-    string sval(val, len);
-    free(val);
+    std::string val;
+    val.resize(len);
+    cdb_read(&d_cdb, &val[0], len, pos);
 
-    value = make_pair(skey, sval);
+    value = make_pair(std::move(key), std::move(val));
     return true;
   }
+
   // We're done searching, so we can clean up d_key
   if (d_searchType != SearchAll) {
-    free(d_key);
+    d_key.clear();
   }
+
   return false;
 }
 
@@ -144,11 +140,11 @@ vector<string> CDB::findall(string &key)
     x++;
     unsigned int vpos = cdb_datapos(&d_cdb);
     unsigned int vlen = cdb_datalen(&d_cdb);
-    char *val = (char *)malloc(vlen);
-    cdb_read(&d_cdb, val, vlen, vpos);
-    string sval(val, vlen);
-    ret.push_back(sval);
-    free(val);
+    std::string val;
+    val.resize(vlen);
+    cdb_read(&d_cdb, &val[0], vlen, vpos);
+    ret.push_back(std::move(val));
   }
+
   return ret;
 }
diff --git a/modules/tinydnsbackend/cdb.hh b/modules/tinydnsbackend/cdb.hh
index 7d5115cc6..f1f85a4c1 100644
--- a/modules/tinydnsbackend/cdb.hh
+++ b/modules/tinydnsbackend/cdb.hh
@@ -22,11 +22,9 @@
 #ifndef CDB_HH
 #define CDB_HH
 
-#include "pdns/logger.hh"
 #include <cdb.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
+
+#include "pdns/misc.hh"
 
 // This class is responsible for the reading of a CDB file.
 // The constructor opens the CDB file, the destructor closes it, so make sure you call that.
@@ -43,13 +41,14 @@ public:
   vector<string> findall(string &key);
 
 private:
-  int d_fd;
   bool moveToNext();
+
+  int d_fd{-1};
   struct cdb d_cdb;
   struct cdb_find d_cdbf;
-  char *d_key;
-  unsigned d_seqPtr;
-  enum SearchType { SearchSuffix, SearchKey, SearchAll } d_searchType;
+  std::string d_key;
+  unsigned d_seqPtr{0};
+  enum SearchType { SearchSuffix, SearchKey, SearchAll } d_searchType{SearchKey};
 };
 
 #endif // CDB_HH
diff --git a/modules/tinydnsbackend/tinydnsbackend.cc b/modules/tinydnsbackend/tinydnsbackend.cc
index 228ac34a5..0eb5adb14 100644
--- a/modules/tinydnsbackend/tinydnsbackend.cc
+++ b/modules/tinydnsbackend/tinydnsbackend.cc
@@ -63,9 +63,14 @@ vector<string> TinyDNSBackend::getLocations()
 
   for (int i=4;i>=0;i--) {
     string searchkey(key, i+2);
-    CDB *reader = new CDB(getArg("dbfile"));
-    ret = reader->findall(searchkey);
-    delete reader;
+    try {
+      auto reader = std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+      ret = reader->findall(searchkey);
+    }
+    catch(const std::exception& e) {
+      g_log<<Logger::Error<<e.what()<<endl;
+      throw PDNSException(e.what());
+    }
 
     //Biggest item wins, so when we find something, we can jump out.
     if (ret.size() > 0) {
@@ -135,7 +140,7 @@ void TinyDNSBackend::getUpdatedMasters(vector<DomainInfo>* retDomains) {
 void TinyDNSBackend::setNotified(uint32_t id, uint32_t serial) {
   Lock l(&s_domainInfoLock);
   if (!s_domainInfo.count(d_suffix)) {
-    throw new PDNSException("Can't get list of domains to set the serial.");
+    throw PDNSException("Can't get list of domains to set the serial.");
   }
   TDI_t *domains = &s_domainInfo[d_suffix];
   TDIById_t& domain_index = domains->get<tag_domainid>();
@@ -153,7 +158,14 @@ void TinyDNSBackend::getAllDomains(vector<DomainInfo> *domains, bool include_dis
   d_isAxfr=true;
   d_dnspacket = NULL;
 
-  d_cdbReader=std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+  try {
+    d_cdbReader=std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+  }
+  catch (const std::exception& e) {
+    g_log<<Logger::Error<<e.what()<<endl;
+    throw PDNSException(e.what());
+  }
+
   d_cdbReader->searchAll();
   DNSResourceRecord rr;
 
@@ -178,7 +190,14 @@ void TinyDNSBackend::getAllDomains(vector<DomainInfo> *domains, bool include_dis
 bool TinyDNSBackend::list(const DNSName &target, int domain_id, bool include_disabled) {
   d_isAxfr=true;
   string key = target.toDNSStringLC();
-  d_cdbReader=std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+  try {
+    d_cdbReader=std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+  }
+  catch (const std::exception& e) {
+    g_log<<Logger::Error<<e.what()<<endl;
+    throw PDNSException(e.what());
+  }
+
   return d_cdbReader->searchSuffix(key);
 }
 
@@ -199,7 +218,14 @@ void TinyDNSBackend::lookup(const QType &qtype, const DNSName &qdomain, DNSPacke
 
   d_qtype=qtype;
 
-  d_cdbReader=std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+  try {
+    d_cdbReader=std::unique_ptr<CDB>(new CDB(getArg("dbfile")));
+  }
+  catch (const std::exception& e) {
+    g_log<<Logger::Error<<e.what()<<endl;
+    throw PDNSException(e.what());
+  }
+
   d_cdbReader->searchKey(key);
   d_dnspacket = pkt_p;
 }