From c6410fa8f01f49b2b96fdf18ea057a8e7537e8a0 Mon Sep 17 00:00:00 2001
From: Tor Egge <Tor.Egge@online.no>
Date: Fri, 19 Nov 2021 20:01:12 +0100
Subject: Detect direct free of element to held buffer.

---
 .../tests/datastore/datastore/datastore_test.cpp   | 52 ++++++++++++++++++++++
 vespalib/src/vespa/vespalib/datastore/datastore.h  |  4 +-
 .../src/vespa/vespalib/datastore/datastore.hpp     | 17 +++----
 3 files changed, 62 insertions(+), 11 deletions(-)

(limited to 'vespalib')

diff --git a/vespalib/src/tests/datastore/datastore/datastore_test.cpp b/vespalib/src/tests/datastore/datastore/datastore_test.cpp
index 2e2728781f4..964978e5510 100644
--- a/vespalib/src/tests/datastore/datastore/datastore_test.cpp
+++ b/vespalib/src/tests/datastore/datastore/datastore_test.cpp
@@ -705,6 +705,58 @@ TEST(DataStoreTest, control_static_sizes) {
     EXPECT_EQ(0, bs.size());
 }
 
+namespace {
+
+void test_free_element_to_held_buffer(bool direct, bool before_hold_buffer)
+{
+    MyStore s;
+    auto ref = s.addEntry(1);
+    EXPECT_EQ(0u, MyRef(ref).bufferId());
+    s.switch_primary_buffer();
+    EXPECT_EQ(1u, s.primary_buffer_id());
+    
+    if (before_hold_buffer) {
+        if (direct) {
+            s.freeElem(ref, 1);
+        } else {
+            s.holdElem(ref, 1);
+        }
+    }
+    s.holdBuffer(0); // hold last buffer
+    if (!before_hold_buffer) {
+        if (direct) {
+            ASSERT_DEATH({ s.freeElem(ref, 1); }, "state.isOnHold\\(\\) && was_held");
+        } else {
+            ASSERT_DEATH({ s.holdElem(ref, 1); }, "state.isActive\\(\\)");
+        }
+    }
+    s.transferHoldLists(100);
+    s.trimHoldLists(101);
+}
+
+}
+
+TEST(DataStoreTest, free_to_active_then_held_buffer_is_ok)
+{
+    test_free_element_to_held_buffer(true, true);
+}
+
+TEST(DataStoreTest, hold_to_active_then_held_buffer_is_ok)
+{
+    test_free_element_to_held_buffer(false, true);
+}
+
+#ifndef NDEBUG
+TEST(DataStoreDeathTest, free_to_held_buffer_is_not_ok)
+{
+    test_free_element_to_held_buffer(true, false);
+}
+
+TEST(DataStoreDeathTest, hold_to_held_buffer_is_not_ok)
+{
+    test_free_element_to_held_buffer(false, false);
+}
+#endif
 
 }
 
diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.h b/vespalib/src/vespa/vespalib/datastore/datastore.h
index d8bb0fb3d0f..be74c2a60d5 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastore.h
+++ b/vespalib/src/vespa/vespalib/datastore/datastore.h
@@ -27,6 +27,8 @@ template <typename RefT = EntryRefT<22> >
 class DataStoreT : public DataStoreBase
 {
 private:
+    void free_elem_internal(EntryRef ref, size_t numElems, bool was_held);
+
 public:
     typedef RefT RefType;
 
@@ -49,7 +51,7 @@ public:
     /**
      * Free element(s).
      */
-    void freeElem(EntryRef ref, size_t numElems);
+    void freeElem(EntryRef ref, size_t numElems) { free_elem_internal(ref, numElems, false); }
 
     /**
      * Hold element(s).
diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.hpp b/vespalib/src/vespa/vespalib/datastore/datastore.hpp
index 81027196a75..23dcb9222a1 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastore.hpp
+++ b/vespalib/src/vespa/vespalib/datastore/datastore.hpp
@@ -23,7 +23,7 @@ DataStoreT<RefT>::~DataStoreT() = default;
 
 template <typename RefT>
 void
-DataStoreT<RefT>::freeElem(EntryRef ref, size_t numElems)
+DataStoreT<RefT>::free_elem_internal(EntryRef ref, size_t numElems, bool was_held)
 {
     RefType intRef(ref);
     BufferState &state = getBufferState(intRef.bufferId());
@@ -35,9 +35,12 @@ DataStoreT<RefT>::freeElem(EntryRef ref, size_t numElems)
             state.freeList().push_back(ref);
         }
     } else {
-        assert(state.isOnHold());
+        assert(state.isOnHold() && was_held);
     }
     state.incDeadElems(numElems);
+    if (was_held) {
+        state.decHoldElems(numElems);
+    }
     state.cleanHold(getBuffer(intRef.bufferId()),
                     intRef.unscaled_offset() * state.getArraySize(), numElems);
 }
@@ -71,10 +74,7 @@ DataStoreT<RefT>::trimElemHoldList(generation_t usedGen)
     for (; it != ite; ++it) {
         if (static_cast<sgeneration_t>(it->_generation - usedGen) >= 0)
             break;
-        RefType intRef(it->_ref);
-        BufferState &state = getBufferState(intRef.bufferId());
-        freeElem(it->_ref, it->_len);
-        state.decHoldElems(it->_len);
+        free_elem_internal(it->_ref, it->_len, true);
         ++freed;
     }
     if (freed != 0) {
@@ -91,10 +91,7 @@ DataStoreT<RefT>::clearElemHoldList()
     ElemHold2List::iterator it(elemHold2List.begin());
     ElemHold2List::iterator ite(elemHold2List.end());
     for (; it != ite; ++it) {
-        RefType intRef(it->_ref);
-        BufferState &state = getBufferState(intRef.bufferId());
-        freeElem(it->_ref, it->_len);
-        state.decHoldElems(it->_len);
+        free_elem_internal(it->_ref, it->_len, true);
     }
     elemHold2List.clear();
 }
-- 
cgit v1.2.3