From 51731f3ad810786d749c29fb18acb75675e1fd35 Mon Sep 17 00:00:00 2001 From: Liyun Xiu Date: Mon, 6 Jul 2026 07:18:12 +0000 Subject: [PATCH] Fix wrapped index not released by IDMapIndex Signed-off-by: Liyun Xiu --- nsparse/id_map_index.cpp | 4 +-- nsparse/id_map_index.h | 8 ++++- tests/id_map_index_test.cpp | 62 +++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/nsparse/id_map_index.cpp b/nsparse/id_map_index.cpp index e821423..f776310 100644 --- a/nsparse/id_map_index.cpp +++ b/nsparse/id_map_index.cpp @@ -75,7 +75,7 @@ void IDMapIndex::add_with_ids(idx_t n, const idx_t* indptr, } } void IDMapIndex::write_index(IOWriter* io_writer) { - nsparse::detail::write_index(delegate_, io_writer, true); + nsparse::detail::write_index(delegate_.get(), io_writer, true); // Write internal_to_external_ vector size_t map_size = internal_to_external_.size(); @@ -86,7 +86,7 @@ void IDMapIndex::write_index(IOWriter* io_writer) { } void IDMapIndex::read_index(IOReader* io_reader) { - delegate_ = nsparse::detail::read_index(io_reader, true); + delegate_.reset(nsparse::detail::read_index(io_reader, true)); // Read internal_to_external_ vector size_t map_size = 0; diff --git a/nsparse/id_map_index.h b/nsparse/id_map_index.h index 4c437c9..defe9de 100644 --- a/nsparse/id_map_index.h +++ b/nsparse/id_map_index.h @@ -11,6 +11,7 @@ #define ID_MAP_INDEX_H #include #include +#include #include #include @@ -78,6 +79,8 @@ class IDMapIndex : public Index, public IndexIO { public: IDMapIndex() = default; static constexpr std::array name = {'I', 'D', 'M', 'P'}; + // Takes ownership of the delegate index; it is freed when this IDMapIndex + // is destroyed. explicit IDMapIndex(Index*); std::array id() const override { return name; } @@ -95,7 +98,10 @@ class IDMapIndex : public Index, public IndexIO { void read_index(IOReader* io_reader) override; private: - Index* delegate_ = nullptr; + // Owns the wrapped delegate index. Using unique_ptr ensures the delegate is + // freed when the IDMapIndex is destroyed (previously a raw pointer with a + // defaulted destructor, which leaked the delegate and everything it owned). + std::unique_ptr delegate_; std::vector internal_to_external_; absl::flat_hash_map external_to_internal_; }; diff --git a/tests/id_map_index_test.cpp b/tests/id_map_index_test.cpp index 7b02f13..0adb36e 100644 --- a/tests/id_map_index_test.cpp +++ b/tests/id_map_index_test.cpp @@ -14,6 +14,10 @@ #include #include "nsparse/id_selector.h" +#include "nsparse/index.h" +#include "nsparse/inverted_index.h" +#include "nsparse/io/buffered_io.h" +#include "nsparse/io/index_io.h" #include "nsparse/seismic_index.h" #include "nsparse/types.h" @@ -274,6 +278,64 @@ TEST_F(IDMapIndexTest, search_with_id_selector_excludes_all) { } } +namespace { + +// Minimal Index used to observe delegate destruction. Sets a caller-owned flag +// to true in its destructor so a test can assert the IDMapIndex frees it. +class DestructionTrackingIndex : public nsparse::Index { +public: + explicit DestructionTrackingIndex(bool* destroyed) + : nsparse::Index(0), destroyed_(destroyed) {} + ~DestructionTrackingIndex() override { *destroyed_ = true; } + + std::array id() const override { return {'T', 'E', 'S', 'T'}; } + void add(nsparse::idx_t, const nsparse::idx_t*, const nsparse::term_t*, + const float*) override {} + +private: + bool* destroyed_; +}; + +} // namespace + +// Bug regression: IDMapIndex used to hold its delegate as a raw pointer with a +// defaulted destructor, leaking the delegate (and everything it owned) on +// destruction. The delegate must now be freed when the IDMapIndex is destroyed. +TEST(IDMapIndexOwnership, DeletesDelegateOnDestruction) { + bool destroyed = false; + { + nsparse::IDMapIndex idmap(new DestructionTrackingIndex(&destroyed)); + EXPECT_FALSE(destroyed); + } + EXPECT_TRUE(destroyed) << "IDMapIndex must delete its delegate index"; +} + +// The delegate acquired during deserialization must also be owned/freed. Before +// the fix this leaked the freshly read delegate index. +TEST(IDMapIndexOwnership, DeletesDelegateAcquiredViaReadIndex) { + // Build and serialize an idmap-wrapped inverted index. + auto* original = new nsparse::IDMapIndex(new nsparse::InvertedIndex(16)); + std::vector indptr = {0, 2, 4}; + std::vector indices = {0, 1, 2, 3}; + std::vector values = {1.0F, 0.5F, 0.8F, 0.3F}; + std::vector ids = {100, 200}; + original->add_with_ids(2, indptr.data(), indices.data(), values.data(), + ids.data()); + original->build(); + + nsparse::BufferedIOWriter writer; + nsparse::write_index(original, &writer); + delete original; // must not leak its delegate + + // read_index constructs a fresh IDMapIndex whose read_index() allocates a + // new delegate; deleting the wrapper must free that delegate too. Under + // ASan/LeakSanitizer this test fails if either delegate leaks. + nsparse::BufferedIOReader reader(writer.data()); + nsparse::Index* loaded = nsparse::read_index(&reader); + ASSERT_NE(loaded, nullptr); + delete loaded; +} + TEST_F(IDMapIndexTest, search_with_not_id_selector) { std::vector indptr = {0, 2, 4, 6}; std::vector indices = {0, 1, 0, 1, 0, 1};