Fix minor memory leaks and enable more tests in CI with sanitizers (#695

) * Avoid leaking buffers after they have been consumed * Enable RNTuple builds for sanitizer CI workflows * Fix leaking of consumed buffers for SIO * Remove tests from ignore list that are no longer present * Disable a few tests again for Thread sanitizer * Disable a few tests for UBSan on clang * Homogenize test names * Minor fix from clang-tidy / clang-format
AIDASoft · Oct 15, 2024 · 2a04151 · 2a04151
1 parent 383c222
commit 2a04151
Show file tree

Hide file tree

Showing 13 changed files with 59 additions and 15 deletions.
diff --git a/.github/workflows/sanitizers.yaml b/.github/workflows/sanitizers.yaml
@@ -44,6 +44,7 @@ jobs:
               -DUSE_EXTERNAL_CATCH2=OFF \
               -DENABLE_SIO=ON \
               -DENABLE_JULIA=ON \
+              -DENABLE_RNTUPLE=ON \
               -G Ninja ..
             echo "::endgroup::"
             echo "::group::Build"

diff --git a/cmake/podioTest.cmake b/cmake/podioTest.cmake
@@ -14,6 +14,7 @@ function(PODIO_SET_TEST_ENV test)
       PODIO_BASE=${PROJECT_SOURCE_DIR}
       ENABLE_SIO=${ENABLE_SIO}
       PODIO_BUILD_BASE=${PROJECT_BINARY_DIR}
+      LSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/tests/root_io/leak_sanitizer_suppressions.txt
   )
   set_property(TEST ${test}
     PROPERTY ENVIRONMENT "${test_environment}"

diff --git a/include/podio/SIOBlockUserData.h b/include/podio/SIOBlockUserData.h
@@ -47,12 +47,11 @@ class SIOBlockUserData : public podio::SIOBlock {
             .createBuffers(podio::userDataCollTypeName<BasicType>(), sio::version::major_version(version), false)
             .value();
 
-    auto* dataVec = new std::vector<BasicType>();
+    auto* dataVec = m_buffers.dataAsVector<BasicType>();
     unsigned size(0);
     device.data(size);
     dataVec->resize(size);
     podio::handlePODDataSIO(device, &(*dataVec)[0], size);
-    m_buffers.data = dataVec;
   }
 
   void write(sio::write_device& device) override {

diff --git a/include/podio/SIOFrameData.h b/include/podio/SIOFrameData.h
@@ -23,7 +23,7 @@ class SIOFrameData {
 
 public:
   SIOFrameData() = delete;
-  ~SIOFrameData() = default;
+  ~SIOFrameData();
 
   SIOFrameData(const SIOFrameData&) = delete;
   SIOFrameData& operator=(const SIOFrameData&) = delete;

diff --git a/include/podio/detail/LinkCollectionData.h b/include/podio/detail/LinkCollectionData.h
@@ -34,6 +34,8 @@ class LinkCollectionData {
     if (!isSubsetColl) {
       m_data.reset(buffers.dataAsVector<LinkData>());
     }
+
+    delete buffers.references;
   }
 
   LinkCollectionData(const LinkCollectionData&) = delete;

diff --git a/python/templates/CollectionData.cc.jinja2 b/python/templates/CollectionData.cc.jinja2
@@ -61,7 +61,9 @@ void {{ class_type }}::clear(bool isSubsetColl) {
   }
 
   // Normal collections manage a bit more and have to clean up a bit more
-  if (m_data) m_data->clear();
+  if (m_data) {
+    m_data->clear();
+  }
 {% if OneToManyRelations or OneToOneRelations %}
   for (auto& pointer : m_refCollections) { pointer->clear(); }
 {% endif %}

diff --git a/src/SIOFrameData.cc b/src/SIOFrameData.cc
@@ -100,4 +100,13 @@ void SIOFrameData::readIdTable() {
   m_subsetCollectionBits = idTableBlock->getSubsetCollectionBits();
 }
 
+SIOFrameData::~SIOFrameData() {
+  for (size_t i = 1; i < m_blocks.size(); ++i) {
+    if (m_availableBlocks[i]) {
+      auto buffers = dynamic_cast<SIOBlock*>(m_blocks[i].get())->getBuffers();
+      buffers.deleteBuffers(buffers);
+    }
+  }
+}
+
 } // namespace podio
diff --git a/src/UserDataCollection.cc b/src/UserDataCollection.cc
@@ -27,7 +27,9 @@ namespace {
               podio::UserDataCollection<T>::schemaVersion,
               podio::userDataCollTypeName<T>(),
               [](podio::CollectionReadBuffers buffers, bool) {
-                return std::make_unique<UserDataCollection<T>>(std::move(*buffers.dataAsVector<T>()));
+                auto vec = std::move(*buffers.dataAsVector<T>());
+                delete static_cast<std::vector<T>*>(buffers.data);
+                return std::make_unique<UserDataCollection<T>>(std::move(vec));
               },
               [](podio::CollectionReadBuffers& buffers) {
                 buffers.data = podio::CollectionWriteBuffers::asVector<T>(buffers.data);

diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake
@@ -30,6 +30,9 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ
     write_python_frame_sio
     read_python_frame_sio
 
+    write_python_frame_rntuple
+    read_python_frame_rntuple
+
     relation_range
 
     pyunittest
@@ -45,10 +48,16 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ
     podio-dump-legacy_sio_v00-16-06
     podio-dump-legacy_sio-detailed_v00-16-06
 
+    podio-dump-rntuple
+    podio-dump-detailed-rntuple
+
     datamodel_def_store_roundtrip_root
     datamodel_def_store_roundtrip_root_extension
     datamodel_def_store_roundtrip_sio
     datamodel_def_store_roundtrip_sio_extension
+    datamodel_def_store_roundtrip_rntuple
+    datamodel_def_store_roundtrip_rntuple_extension
+
 
     write_old_data_root
     read_new_data_root
@@ -76,15 +85,30 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ
     set(CTEST_CUSTOM_TESTS_IGNORE
       ${CTEST_CUSTOM_TESTS_IGNORE}
 
-      read_sio
-      read_and_write_sio
-      write_timed_sio
-      read_timed_sio
-      read_frame_sio
-      read_interface_sio
       read_frame_legacy_sio
       read_and_write_frame_sio
       )
   endif()
 
+  if("@USE_SANITIZER@" MATCHES "Thread")
+    set(CTEST_CUSTOM_TESTS_IGNORE
+      ${CTEST_CUSTOM_TESTS_IGNORE}
+
+      read_rntuple
+      read_interface_rntuple
+    )
+  endif()
+
+  if("@USE_SANITIZER@" MATCHES "Undefined" AND "@CMAKE_CXX_COMPILER_ID@" STREQUAL "Clang")
+    set(CTEST_CUSTOM_TESTS_IGNORE
+      ${CTEST_CUSTOM_TESTS_IGNORE}
+
+      write_rntuple
+      read_rntuple
+      write_interface_rntuple
+      read_interface_rntuple
+    )
+
+  endif()
+
 endif()
diff --git a/tests/root_io/leak_sanitizer_suppressions.txt b/tests/root_io/leak_sanitizer_suppressions.txt
@@ -0,0 +1,2 @@
+# Ignore leaks from Cling
+leak:Cling
diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt
@@ -83,5 +83,7 @@ else()
         PODIO_SIOBLOCK_PATH=${PROJECT_BINARY_DIR}/tests
         ENVIRONMENT
         LD_LIBRARY_PATH=${CMAKE_CURRENT_BINARY_DIR}:${PROJECT_BINARY_DIR}/src:${PROJECT_BINARY_DIR}/tests:$<TARGET_FILE_DIR:ROOT::Tree>:$<$<TARGET_EXISTS:SIO::sio>:$<TARGET_FILE_DIR:SIO::sio>>:$ENV{LD_LIBRARY_PATH}
+      ENVIRONMENT
+      LSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/tests/root_io/leak_sanitizer_suppressions.txt
   )
 endif()
diff --git a/tests/unittests/unittest.cpp b/tests/unittests/unittest.cpp
@@ -1380,16 +1380,16 @@ TEST_CASE("ROOTWriter check consistency", "[ASAN-FAIL][UBSAN-FAIL][basics][root]
 
 #if PODIO_ENABLE_RNTUPLE
 
-TEST_CASE("Relations after cloning with RNTuple", "[relations][basics]") {
+TEST_CASE("Relations after cloning with RNTuple", "[THREAD-FAIL][UBSAN-FAIL][relations][basics]") {
   runRelationAfterCloneCheck<podio::RNTupleReader, podio::RNTupleWriter>(
       "unittests_relations_after_cloning_rntuple.root");
 }
 
-TEST_CASE("RNTupleWriter consistent frame contents", "[basics][root]") {
+TEST_CASE("RNTupleWriter consistent frame contents", "[UBSAN-FAIL][basics][root]") {
   runConsistentFrameTest<podio::RNTupleWriter>("unittests_frame_consistency_rntuple.root");
 }
 
-TEST_CASE("RNTupleWriter check consistency", "[basics][root]") {
+TEST_CASE("RNTupleWriter check consistency", "[UBSAN-FAIL][basics][root]") {
   runCheckConsistencyTest<podio::RNTupleWriter>("unittests_frame_check_consistency_rntuple.root");
 }
 

diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
@@ -94,7 +94,7 @@ if(BUILD_TESTING)
 
   if (ENABLE_RNTUPLE)
     CREATE_DUMP_TEST(podio-dump-rntuple "write_rntuple" ${PROJECT_BINARY_DIR}/tests/root_io/example_rntuple.root)
-    CREATE_DUMP_TEST(podio-dump-rntuple-detailed "write_rntuple" --detailed --category events --entries 1:3 ${PROJECT_BINARY_DIR}/tests/root_io/example_rntuple.root)
+    CREATE_DUMP_TEST(podio-dump-detailed-rntuple "write_rntuple" --detailed --category events --entries 1:3 ${PROJECT_BINARY_DIR}/tests/root_io/example_rntuple.root)
   endif()
 
 endif()