From 689eca3f8a00cafb54ed473e6ad684f155d9ac45 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Thu, 19 Mar 2026 14:49:06 -0700 Subject: [PATCH] refactor: replace raw new/delete with std::unique_ptr in NRadix Replace manual memory management with RAII using std::unique_ptr for: - nRadixNode members (value_, left_, right_) - NRadixTree root_ member This eliminates manual delete calls, simplifies copy/assignment operators, and prevents potential memory leaks. Co-Authored-By: Claude Sonnet 4.5 --- collector/lib/NRadix.cpp | 45 ++++++++++++++++++++-------------------- collector/lib/NRadix.h | 40 ++++++++++++++--------------------- 2 files changed, 39 insertions(+), 46 deletions(-) diff --git a/collector/lib/NRadix.cpp b/collector/lib/NRadix.cpp index 911be3b140..de016f9eb2 100644 --- a/collector/lib/NRadix.cpp +++ b/collector/lib/NRadix.cpp @@ -48,17 +48,17 @@ bool NRadixTree::Insert(const IPNet& network) const { const uint64_t* net_mask_p = net_mask.data(); uint64_t bit(0x8000000000000000ULL); - nRadixNode* node = this->root_; - nRadixNode* next = this->root_; + nRadixNode* node = this->root_.get(); + nRadixNode* next = this->root_.get(); size_t i = 0; // Traverse the tree for the bits that already exist in the tree. while (bit & *net_mask_p) { // If the bit is set, go right, otherwise left. if (ntohll(*addr_p) & bit) { - next = node->right_; + next = node->right_.get(); } else { - next = node->left_; + next = node->left_.get(); } if (!next) { @@ -90,18 +90,19 @@ bool NRadixTree::Insert(const IPNet& network) const { CLOG(ERROR) << "CIDR " << network << " already exists"; return false; } - node->value_ = new IPNet(network); + node->value_ = std::make_unique(network); return true; } // There still are bits to be walked, so go ahead and add them to the tree. while (bit & *net_mask_p) { - next = new nRadixNode(); + auto new_node = std::make_unique(); + next = new_node.get(); if (ntohll(*addr_p) & bit) { - node->right_ = next; + node->right_ = std::move(new_node); } else { - node->left_ = next; + node->left_ = std::move(new_node); } bit >>= 1; @@ -121,7 +122,7 @@ bool NRadixTree::Insert(const IPNet& network) const { } } - node->value_ = new IPNet(network); + node->value_ = std::make_unique(network); return true; } @@ -141,7 +142,7 @@ IPNet NRadixTree::Find(const IPNet& network) const { uint64_t bit(0x8000000000000000ULL); IPNet ret; - nRadixNode* node = this->root_; + nRadixNode* node = this->root_.get(); size_t i = 0; while (node) { if (node->value_) { @@ -149,9 +150,9 @@ IPNet NRadixTree::Find(const IPNet& network) const { } if (ntohll(*addr_p) & bit) { - node = node->right_; + node = node->right_.get(); } else { - node = node->left_; + node = node->left_.get(); } // All network bits are traversed. If a supernet was found along the way, `ret` holds it, @@ -195,13 +196,13 @@ void getAll(nRadixNode* node, std::vector& ret) { ret.push_back(*node->value_); } - getAll(node->left_, ret); - getAll(node->right_, ret); + getAll(node->left_.get(), ret); + getAll(node->right_.get(), ret); } std::vector NRadixTree::GetAll() const { std::vector ret; - getAll(this->root_, ret); + getAll(this->root_.get(), ret); return ret; } @@ -227,11 +228,11 @@ bool isAnyIPNetSubsetUtil(Address::Family family, const nRadixNode* n1, const nR } if (n1 && n1->value_) { - containing_net = n1->value_; + containing_net = n1->value_.get(); } if (n2->value_) { - contained_net = n2->value_; + contained_net = n2->value_.get(); } // If we find a network in first tree, that means it contains @@ -240,11 +241,11 @@ bool isAnyIPNetSubsetUtil(Address::Family family, const nRadixNode* n1, const nR // finding the smaller network down the path. if (n1) { - return isAnyIPNetSubsetUtil(family, n1->left_, n2->left_, containing_net, contained_net) || - isAnyIPNetSubsetUtil(family, n1->right_, n2->right_, containing_net, contained_net); + return isAnyIPNetSubsetUtil(family, n1->left_.get(), n2->left_.get(), containing_net, contained_net) || + isAnyIPNetSubsetUtil(family, n1->right_.get(), n2->right_.get(), containing_net, contained_net); } - return isAnyIPNetSubsetUtil(family, nullptr, n2->left_, containing_net, contained_net) || - isAnyIPNetSubsetUtil(family, nullptr, n2->right_, containing_net, contained_net); + return isAnyIPNetSubsetUtil(family, nullptr, n2->left_.get(), containing_net, contained_net) || + isAnyIPNetSubsetUtil(family, nullptr, n2->right_.get(), containing_net, contained_net); } bool NRadixTree::IsEmpty() const { @@ -256,7 +257,7 @@ bool NRadixTree::IsAnyIPNetSubset(const NRadixTree& other) const { } bool NRadixTree::IsAnyIPNetSubset(Address::Family family, const NRadixTree& other) const { - return isAnyIPNetSubsetUtil(family, root_, other.root_, nullptr, nullptr); + return isAnyIPNetSubsetUtil(family, root_.get(), other.root_.get(), nullptr, nullptr); } } // namespace collector diff --git a/collector/lib/NRadix.h b/collector/lib/NRadix.h index 4c3baeca8f..7f2a3dcea9 100644 --- a/collector/lib/NRadix.h +++ b/collector/lib/NRadix.h @@ -28,6 +28,7 @@ #pragma once +#include #include #include "Logging.h" @@ -38,17 +39,17 @@ namespace collector { struct nRadixNode { nRadixNode() : value_(nullptr), left_(nullptr), right_(nullptr) {} - explicit nRadixNode(const IPNet& value) : value_(new IPNet(value)), left_(nullptr), right_(nullptr) {} + explicit nRadixNode(const IPNet& value) : value_(std::make_unique(value)), left_(nullptr), right_(nullptr) {} nRadixNode(const nRadixNode& other) : value_(nullptr), left_(nullptr), right_(nullptr) { if (other.value_) { - value_ = new IPNet(*other.value_); + value_ = std::make_unique(*other.value_); } if (other.left_) { - left_ = new nRadixNode(*other.left_); + left_ = std::make_unique(*other.left_); } if (other.right_) { - right_ = new nRadixNode(*other.right_); + right_ = std::make_unique(*other.right_); } } @@ -56,27 +57,22 @@ struct nRadixNode { if (this == &other) { return *this; } - auto* new_node = new nRadixNode(other); + auto new_node = std::make_unique(other); std::swap(*new_node, *this); - delete new_node; return *this; } - ~nRadixNode() { - delete left_; - delete right_; - delete value_; - } + ~nRadixNode() = default; - const IPNet* value_; - nRadixNode* left_; - nRadixNode* right_; + std::unique_ptr value_; + std::unique_ptr left_; + std::unique_ptr right_; }; class NRadixTree { public: - NRadixTree() : root_(new nRadixNode()) {} - explicit NRadixTree(const std::vector& networks) : root_(new nRadixNode()) { + NRadixTree() : root_(std::make_unique()) {} + explicit NRadixTree(const std::vector& networks) : root_(std::make_unique()) { for (const auto& network : networks) { auto inserted = this->Insert(network); if (!inserted) { @@ -85,20 +81,16 @@ class NRadixTree { } } - NRadixTree(const NRadixTree& other) : root_(new nRadixNode(*other.root_)) {} + NRadixTree(const NRadixTree& other) : root_(std::make_unique(*other.root_)) {} - ~NRadixTree() { - // This calls the node destructor which in turn cleans up all the nodes. - delete root_; - } + ~NRadixTree() = default; NRadixTree& operator=(const NRadixTree& other) { if (this == &other) { return *this; } - delete root_; // This calls the node copy constructor which in turn copies all the nodes. - root_ = new nRadixNode(*other.root_); + root_ = std::make_unique(*other.root_); return *this; } @@ -120,7 +112,7 @@ class NRadixTree { // Determines whether any network in `other` is fully contained by any network in this tree, for a given family. bool IsAnyIPNetSubset(Address::Family family, const NRadixTree& other) const; - nRadixNode* root_; + std::unique_ptr root_; }; } // namespace collector