From 9772e56de4d79bc6b3418d789ed8ac46119542fc Mon Sep 17 00:00:00 2001
From: Stephan Bosch <stephan.bosch@open-xchange.com>
Date: Mon, 30 Sep 2019 10:30:25 +0200
Subject: [PATCH] iputils.hh: NetmaskTree: Make tree cleanup mandatory.

Potentially leaving branches full of unassigned nodes unnecessarily complicates
tree algorithms. Disabling tree cleanup was not used anywhere, except for a unit
test. Note that, after this change, individual branch nodes can still be
unassigned, but not the whole branch. So, when e.g. the left sub-branch of a
node exists, algorithms can rely on the fact that there is at least one assigned
node in there.
---
 pdns/iputils.hh         | 20 ++++----------------
 pdns/recursor_cache.hh  |  2 +-
 pdns/test-iputils_hh.cc |  2 +-
 3 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/pdns/iputils.hh b/pdns/iputils.hh
index ea97030764..9a66e13b84 100644
--- a/pdns/iputils.hh
+++ b/pdns/iputils.hh
@@ -790,13 +790,10 @@ private:
   }
 
 public:
-  NetmaskTree() noexcept : NetmaskTree(false) {
+  NetmaskTree() noexcept : d_root(new TreeNode()) {
   }
 
-  NetmaskTree(bool cleanup) noexcept : d_root(new TreeNode()), d_cleanup_tree(cleanup) {
-  }
-
-  NetmaskTree(const NetmaskTree& rhs): d_root(new TreeNode()), d_cleanup_tree(rhs.d_cleanup_tree) {
+  NetmaskTree(const NetmaskTree& rhs): d_root(new TreeNode()) {
     // it is easier to copy the nodes than tree.
     // also acts as handy compactor
     for(auto const& node: rhs._nodes)
@@ -808,7 +805,6 @@ public:
     // see above.
     for(auto const& node: rhs._nodes)
       insert(node->first).second = node->second;
-    d_cleanup_tree = rhs.d_cleanup_tree;
     return *this;
   }
 
@@ -1031,9 +1027,7 @@ public:
       _nodes.erase(node->node.get());
       node->assigned = false;
       node->node->second = value_type();
-
-      if (d_cleanup_tree)
-        cleanup_tree(node);
+      cleanup_tree(node);
     }
   }
 
@@ -1075,7 +1069,6 @@ public:
 private:
   unique_ptr<TreeNode> d_root; //<! Root of our tree
   std::set<node_type*> _nodes; //<! Container for actual values
-  bool d_cleanup_tree; //<! Whether or not to cleanup the tree on erase
 };
 
 /** This class represents a group of supplemental Netmask classes. An IP address matchs
@@ -1084,12 +1077,7 @@ private:
 class NetmaskGroup
 {
 public:
-  //! By default, initialise the tree to cleanup
-  NetmaskGroup() noexcept : NetmaskGroup(true) {
-  }
-
-  //! This allows control over whether to cleanup or not
-  NetmaskGroup(bool cleanup) noexcept : tree(cleanup) {
+  NetmaskGroup() noexcept {
   }
 
   //! If this IP address is matched by any of the classes within
diff --git a/pdns/recursor_cache.hh b/pdns/recursor_cache.hh
index d298671ba0..00348c4b57 100644
--- a/pdns/recursor_cache.hh
+++ b/pdns/recursor_cache.hh
@@ -107,7 +107,7 @@ private:
   class ECSIndexEntry
   {
   public:
-    ECSIndexEntry(const DNSName& qname, uint16_t qtype): d_nmt(true), d_qname(qname), d_qtype(qtype)
+    ECSIndexEntry(const DNSName& qname, uint16_t qtype): d_nmt(), d_qname(qname), d_qtype(qtype)
     {
     }
 
diff --git a/pdns/test-iputils_hh.cc b/pdns/test-iputils_hh.cc
index 5820c87d6f..dc14bf558d 100644
--- a/pdns/test-iputils_hh.cc
+++ b/pdns/test-iputils_hh.cc
@@ -478,7 +478,7 @@ BOOST_AUTO_TEST_CASE(test_scale) {
 
 BOOST_AUTO_TEST_CASE(test_removal) {
   std::string prefix = "192.";
-  NetmaskTree<int> nmt(true);
+  NetmaskTree<int> nmt;
   BOOST_CHECK(nmt.empty());
   BOOST_CHECK_EQUAL(nmt.size(), 0);
 
-- 
2.47.2