From 2a04151bb53728c25a405a02e8fb1fc9d078790d Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Tue, 15 Oct 2024 16:43:08 +0200 Subject: [PATCH] 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 --- .github/workflows/sanitizers.yaml | 1 + cmake/podioTest.cmake | 1 + include/podio/SIOBlockUserData.h | 3 +- include/podio/SIOFrameData.h | 2 +- include/podio/detail/LinkCollectionData.h | 2 ++ python/templates/CollectionData.cc.jinja2 | 4 ++- src/SIOFrameData.cc | 9 +++++ src/UserDataCollection.cc | 4 ++- tests/CTestCustom.cmake | 36 +++++++++++++++---- tests/root_io/leak_sanitizer_suppressions.txt | 2 ++ tests/unittests/CMakeLists.txt | 2 ++ tests/unittests/unittest.cpp | 6 ++-- tools/CMakeLists.txt | 2 +- 13 files changed, 59 insertions(+), 15 deletions(-) create mode 100644 tests/root_io/leak_sanitizer_suppressions.txt diff --git a/.github/workflows/sanitizers.yaml b/.github/workflows/sanitizers.yaml index afeefa15e..da47f859f 100644 --- 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 index 0f2b9c675..4c4d8f24d 100644 --- 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 index 4d3110fba..5921ef018 100644 --- a/include/podio/SIOBlockUserData.h +++ b/include/podio/SIOBlockUserData.h @@ -47,12 +47,11 @@ class SIOBlockUserData : public podio::SIOBlock { .createBuffers(podio::userDataCollTypeName(), sio::version::major_version(version), false) .value(); - auto* dataVec = new std::vector(); + auto* dataVec = m_buffers.dataAsVector(); 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 index 95b09d927..dd2dddb20 100644 --- 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 index 948d7247d..7c8c2d925 100644 --- 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()); } + + delete buffers.references; } LinkCollectionData(const LinkCollectionData&) = delete; diff --git a/python/templates/CollectionData.cc.jinja2 b/python/templates/CollectionData.cc.jinja2 index 6cf76fcbe..f31491e7a 100644 --- 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 index 806639672..5615463a0 100644 --- 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(m_blocks[i].get())->getBuffers(); + buffers.deleteBuffers(buffers); + } + } +} + } // namespace podio diff --git a/src/UserDataCollection.cc b/src/UserDataCollection.cc index 673e7969c..03b58770b 100644 --- a/src/UserDataCollection.cc +++ b/src/UserDataCollection.cc @@ -27,7 +27,9 @@ namespace { podio::UserDataCollection::schemaVersion, podio::userDataCollTypeName(), [](podio::CollectionReadBuffers buffers, bool) { - return std::make_unique>(std::move(*buffers.dataAsVector())); + auto vec = std::move(*buffers.dataAsVector()); + delete static_cast*>(buffers.data); + return std::make_unique>(std::move(vec)); }, [](podio::CollectionReadBuffers& buffers) { buffers.data = podio::CollectionWriteBuffers::asVector(buffers.data); diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index 802bbed3a..bce6fc5fd 100644 --- 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 new file mode 100644 index 000000000..fe5748c70 --- /dev/null +++ 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 index 926b0fe8c..07c304a57 100644 --- 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:$:$<$:$>:$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 index 9aded5e2c..5f110ded8 100644 --- 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( "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("unittests_frame_consistency_rntuple.root"); } -TEST_CASE("RNTupleWriter check consistency", "[basics][root]") { +TEST_CASE("RNTupleWriter check consistency", "[UBSAN-FAIL][basics][root]") { runCheckConsistencyTest("unittests_frame_check_consistency_rntuple.root"); } diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt index 8c10f98c2..c5fa5d4d8 100644 --- 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()