From 27a7e9b2aa84ad006fe2178c74689fc09820ee97 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:36:52 -0700 Subject: [PATCH 1/7] Removing use of logging in remove_class() functions; fixing comments --- library/utility/indexed_factory.hh | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/library/utility/indexed_factory.hh b/library/utility/indexed_factory.hh index 5fd3a56..82d0c38 100644 --- a/library/utility/indexed_factory.hh +++ b/library/utility/indexed_factory.hh @@ -211,7 +211,8 @@ namespace scarab template< class XIndexType, class XBaseType, typename ... XArgs > void indexed_factory< XIndexType, XBaseType, XArgs... >::register_class( const XIndexType& a_index, const base_registrar< XBaseType, XArgs... >* a_registrar ) { - // A local (non-static) logger is created inside this function to avoid static initialization order problems + // A function-local static logger is used so that it is initialized on first use, + // avoiding static initialization order problems. LOGGER( slog_ind_factory_reg, "indexed_factory_register"); std::unique_lock< std::mutex > t_lock( this->f_factory_mutex ); @@ -234,17 +235,8 @@ namespace scarab template< class XIndexType, class XBaseType, typename ... XArgs > void indexed_factory< XIndexType, XBaseType, XArgs... >::remove_class(const XIndexType& a_index ) { - // A local (non-static) logger is created inside this function to avoid static destruction problems - LOGGER( slog_ind_factory_rem, "indexed_factory_remove"); - LTRACE( slog_ind_factory_rem, "Removing indexed_factory for class " << a_index << " from " << this ); - /* -#ifndef NDEBUG - if( ELevel::eTrace >= f_global_threshold ) - { - std::cout << "Removing indexed_factory for class " << a_index << " from " << this << std::endl; - } -#endif - */ + // No logging here: remove_class() is called from ~indexed_registrar() during static + // destruction, when a static logger may already have been destroyed. FactoryIt iter = fMap->find( a_index ); if( iter != fMap->end() ) fMap->erase( iter ); return; @@ -352,7 +344,8 @@ namespace scarab template< class XIndexType, class XBaseType > void indexed_factory< XIndexType, XBaseType, void >::register_class( const XIndexType& a_index, const base_registrar< XBaseType >* a_registrar ) { - // A local (non-static) logger is created inside this function to avoid static initialization order problems + // A function-local static logger is used so that it is initialized on first use, + // avoiding static initialization order problems. LOGGER( slog_ind_factory_reg, "indexed_factory_register"); std::unique_lock< std::mutex > t_lock( this->f_factory_mutex ); @@ -376,15 +369,8 @@ namespace scarab template< class XIndexType, class XBaseType > void indexed_factory< XIndexType, XBaseType, void >::remove_class(const XIndexType& a_index ) { - // A local (non-static) logger is created inside this function to avoid static destruction problems - LOGGER( slog_ind_factory_rem, "indexed_factory_remove"); - LTRACE( slog_ind_factory_rem, "Removing indexed_factory for class " << a_index << " from " << this ); -//#ifndef NDEBUG -// if( ELevel::eTrace >= f_global_threshold ) -// { -// std::cout << "Removing indexed_factory for class " << a_index << " from " << this << std::endl; -// } -//#endif + // No logging here: remove_class() is called from ~indexed_registrar() during static + // destruction, when a static logger may already have been destroyed. FactoryIt iter = fMap->find( a_index ); if( iter != fMap->end() ) fMap->erase( iter ); return; From c5f0b1ce3a13042506191a9a5ff5b40f263ff66c Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:37:31 -0700 Subject: [PATCH 2/7] Adding static destruction tests and integrating with new GHA job --- .github/workflows/run_tests.yaml | 51 ++++++++++++++++++- testing/CMakeLists.txt | 9 +++- testing/applications/CMakeLists.txt | 12 +++++ .../applications/test_static_destruction.cc | 50 ++++++++++++++++++ 4 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 testing/applications/test_static_destruction.cc diff --git a/.github/workflows/run_tests.yaml b/.github/workflows/run_tests.yaml index bf7dc14..97906f7 100644 --- a/.github/workflows/run_tests.yaml +++ b/.github/workflows/run_tests.yaml @@ -103,13 +103,62 @@ jobs: # if: ${{ ! success() }} # uses: mxschmitt/action-tmate@v3 + RunTestsSanitizers: + + # A separate job rather than part of the RunTests matrix: the sanitizers roughly double + # the build time and need a different ctest invocation, and there is no reason to slow + # down all five platforms. One Linux runner is enough -- the memory errors these catch + # are platform-independent. + runs-on: ubuntu-24.04 + + steps: + + - name: Checkout the repo + uses: actions/checkout@v4 + with: + submodules: recursive + + - name: Install dependencies + run: | + sudo apt-get update + DEBIAN_FRONTEND=noninteractive sudo apt-get install -y \ + libboost-all-dev \ + rapidjson-dev \ + libyaml-cpp-dev + + - name: Configure + # The python bindings are off because running the sanitizers through the python + # interpreter requires preloading the sanitizer runtime, and adds no coverage here. + # CMAKE_BUILD_TYPE is deliberately left unset so that it defaults to DEBUG: NDEBUG + # must stay undefined, because LTRACE and LDEBUG compile to nothing when it is set. + run: | + mkdir build + cd build + cmake .. -DScarab_ENABLE_TESTING=TRUE -DScarab_ENABLE_SANITIZERS=TRUE -DScarab_BUILD_PYTHON=FALSE + + - name: Build + run: | + cd build + make -j2 install + + - name: Run tests + # abort_on_error makes a sanitizer report fail the test rather than only printing. + # Leak detection is off: this job targets memory-safety errors, and the existing + # tests have not been audited for leaks. + env: + ASAN_OPTIONS: abort_on_error=1:detect_leaks=0 + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1 + run: | + cd build + ctest --output-on-failure + Release: runs-on: ubuntu-22.04 if: ${{ github.event_name == 'push' && contains(github.ref, 'refs/tags/') }} - needs: [RunTests] + needs: [RunTests, RunTestsSanitizers] steps: diff --git a/testing/CMakeLists.txt b/testing/CMakeLists.txt index 4fb80fb..de2245b 100644 --- a/testing/CMakeLists.txt +++ b/testing/CMakeLists.txt @@ -99,13 +99,20 @@ set( testing_LIB_DEPENDENCIES Scarab ) -pbuilder_executable( +pbuilder_executable( EXECUTABLE run_tests SOURCES ${testing_SOURCES} PROJECT_LIBRARIES ${testing_LIB_DEPENDENCIES} PRIVATE_EXTERNAL_LIBRARIES Catch2::Catch2 ) +# LINK_FLAGS is used rather than target_link_options because the latter requires CMake 3.13, +# and cmake_minimum_required is 3.12. +if( Scarab_ENABLE_SANITIZERS ) + target_compile_options( run_tests PRIVATE -fsanitize=address,undefined -g ) + set_property( TARGET run_tests APPEND_STRING PROPERTY LINK_FLAGS " -fsanitize=address,undefined" ) +endif() + ########## # Other tests diff --git a/testing/applications/CMakeLists.txt b/testing/applications/CMakeLists.txt index 586a1c5..3d86f94 100644 --- a/testing/applications/CMakeLists.txt +++ b/testing/applications/CMakeLists.txt @@ -12,6 +12,7 @@ set( testing_applications_SOURCES test_raise_sigint.cc test_raise_sigquit.cc test_raise_sigterm.cc + test_static_destruction.cc test_static_initialization.cc test_unhandled_exception.cc ) @@ -20,6 +21,16 @@ pbuilder_executables( SOURCES ${testing_applications_SOURCES} TARGETS_VAR programs PROJECT_LIBRARIES ${testing_LIB_DEPENDENCIES} ) + +# LINK_FLAGS is used rather than target_link_options because the latter requires CMake 3.13, +# and cmake_minimum_required is 3.12. +if( Scarab_ENABLE_SANITIZERS ) + foreach( program ${programs} ) + target_compile_options( ${program} PRIVATE -fsanitize=address,undefined -g ) + set_property( TARGET ${program} APPEND_STRING PROPERTY LINK_FLAGS " -fsanitize=address,undefined" ) + endforeach() +endif() + set( programs ${programs} PARENT_SCOPE ) if( Scarab_ENABLE_TESTING ) @@ -33,6 +44,7 @@ if( Scarab_ENABLE_TESTING ) add_test( NAME raise_sigint COMMAND test_raise_sigint WORKING_DIRECTORY ${BIN_INSTALL_DIR} ) add_test( NAME raise_sigquit COMMAND test_raise_sigquit WORKING_DIRECTORY ${BIN_INSTALL_DIR} ) add_test( NAME raise_sigterm COMMAND test_raise_sigterm WORKING_DIRECTORY ${BIN_INSTALL_DIR} ) + add_test( NAME static_destruction COMMAND test_static_destruction WORKING_DIRECTORY ${BIN_INSTALL_DIR} ) add_test( NAME static_initialization COMMAND test_static_initialization WORKING_DIRECTORY ${BIN_INSTALL_DIR} ) add_test( NAME unhandled_exception COMMAND test_unhandled_exception WORKING_DIRECTORY ${BIN_INSTALL_DIR} ) diff --git a/testing/applications/test_static_destruction.cc b/testing/applications/test_static_destruction.cc new file mode 100644 index 0000000..54c8508 --- /dev/null +++ b/testing/applications/test_static_destruction.cc @@ -0,0 +1,50 @@ +/* + * test_static_destruction.cc + * + * Created on: Sep 4, 2026 + * Author: N.S. Oblath + * + * Use: + * > bin/test_static_destruction + * + * Regression test for a heap-use-after-free during static destruction. + * + * A registrar un-registers itself in its destructor, calling indexed_factory::remove_class(). + * For a static registrar that happens during static destruction, so remove_class() must not + * log: a logger is itself a static object and may already have been destroyed. + * + * Two registrars are required to reproduce the failure. Statics are destroyed in reverse + * order of construction, so a logger created inside remove_class() during the destruction + * phase is destroyed almost immediately afterward -- the first registrar creates it, and + * the second then writes into freed memory. With only one registrar nothing follows it, + * and the bug does not appear. + * + * This program exits successfully either way unless it is built with the sanitizers + * (Scarab_ENABLE_SANITIZERS=TRUE), which is what turns the invalid write into a failure. + * It must also be built without NDEBUG, since LTRACE compiles to nothing when NDEBUG is set. + */ + +#include "factory.hh" + +#include + +namespace scarab +{ + struct test_base + { + virtual ~test_base() {} + }; + + struct test_derived_1 : test_base {}; + struct test_derived_2 : test_base {}; +} + +// Both registrars must be static and use the same factory instantiation so that the +// second one's destructor runs after the first one has already created the logger. +static scarab::registrar< scarab::test_base, scarab::test_derived_1 > s_reg_1( "test_derived_1" ); +static scarab::registrar< scarab::test_base, scarab::test_derived_2 > s_reg_2( "test_derived_2" ); + +int main(int , char ** ) +{ + return( EXIT_SUCCESS ); +} From 5b8f6c7586fdadab3be3059d7bbff293907d9291 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:38:11 -0700 Subject: [PATCH 3/7] Switch to enable address sanitizer testing --- CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 13fe3f6..aa80e48 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -44,6 +44,10 @@ set( Scarab_LOGGER_DEFAULT_THRESHOLD "eTrace" CACHE STRING "Default threshold fo option( Scarab_ENABLE_LOGGER_DEBUG "Flag to enable debug printing for the logger itself" FALSE ) +# Applies to the test programs only, so that the installed library's ABI is unchanged. +# The sanitizer runtime is interposed process-wide, so errors inside the library are still caught. +option( Scarab_ENABLE_SANITIZERS "Flag to build the test programs with the address and UB sanitizers" FALSE ) + if( (Scarab_BUILD_CODEC_JSON OR Scarab_BUILD_CODEC_YAML OR Scarab_BUILD_AUTHENTICATION OR Scarab_BUILD_CLI) AND NOT Scarab_BUILD_PARAM ) message( FATAL_ERROR "Invalid combination of build options. Building the Codecs, Authentication, and CLI requires Param. If you want these elements, turn on Param. If you want Param off, turn these elements off." ) endif() From acab64496c880f75f9f12b1b50358ab382e4ab47 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:38:27 -0700 Subject: [PATCH 4/7] Updating changelog and bumping version --- VERSION | 2 +- changelog.md | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 43a6c02..1b2c289 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3 14 3 +3 14 4 diff --git a/changelog.md b/changelog.md index b54929f..2a56e3b 100644 --- a/changelog.md +++ b/changelog.md @@ -10,6 +10,19 @@ Types of changes: Added, Changed, Deprecated, Removed, Fixed, Security ## [Unreleased] +## [3.14.4] - 2026-09-04 + +### Added + +- Static destruction test for indexed_factory and indexed_registrar +- Scarab_ENABLE_SANITIZERS CMake option to build the test programs with the address and UB sanitizers, and a CI job that uses it + +### Fixed + +- Removed the trace logging from `indexed_factory::remove_class()`, which caused a heap-use-after-free during static destruction: `remove_class()` is called from `~indexed_registrar()`, at which point the static logger it created may already have been destroyed +- Corrected misleading comments in `indexed_factory::register_class()` claiming a local (non-static) logger was used + + ## [3.14.3] - 2026-08-21 ### Fixed From 300976ea3651c3fe4c3b9fd8e4c5ce16b5b722e5 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:49:24 -0700 Subject: [PATCH 5/7] Fix false positive issue in ASan tests --- .github/workflows/run_tests.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run_tests.yaml b/.github/workflows/run_tests.yaml index 97906f7..5be8022 100644 --- a/.github/workflows/run_tests.yaml +++ b/.github/workflows/run_tests.yaml @@ -145,8 +145,12 @@ jobs: # abort_on_error makes a sanitizer report fail the test rather than only printing. # Leak detection is off: this job targets memory-safety errors, and the existing # tests have not been audited for leaks. + # Container-overflow detection is off because it gives false positives unless the + # whole standard library is also built with the sanitizer; param_array's deque of + # unique_ptrs trips it. See + # https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow env: - ASAN_OPTIONS: abort_on_error=1:detect_leaks=0 + ASAN_OPTIONS: abort_on_error=1:detect_leaks=0:detect_container_overflow=0 UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1 run: | cd build From 5d33fbcf16cf6fe7f16306854b53c80aa1522736 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:57:00 -0700 Subject: [PATCH 6/7] Updated GHA checkout to v7 --- .github/workflows/run_tests.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/run_tests.yaml b/.github/workflows/run_tests.yaml index 5be8022..8b5af08 100644 --- a/.github/workflows/run_tests.yaml +++ b/.github/workflows/run_tests.yaml @@ -22,7 +22,7 @@ jobs: steps: - name: Checkout the repo - uses: actions/checkout@v4 + uses: actions/checkout@v7 with: submodules: recursive @@ -114,7 +114,7 @@ jobs: steps: - name: Checkout the repo - uses: actions/checkout@v4 + uses: actions/checkout@v7 with: submodules: recursive @@ -167,7 +167,7 @@ jobs: steps: - name: Checkout the repo - uses: actions/checkout@v4 + uses: actions/checkout@v7 with: submodules: recursive From 58a530a0a70469697cdc7417c23f84c141ba9a57 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Fri, 4 Sep 2026 16:58:51 -0700 Subject: [PATCH 7/7] Updated changelog --- changelog.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changelog.md b/changelog.md index 2a56e3b..f5253e7 100644 --- a/changelog.md +++ b/changelog.md @@ -17,6 +17,10 @@ Types of changes: Added, Changed, Deprecated, Removed, Fixed, Security - Static destruction test for indexed_factory and indexed_registrar - Scarab_ENABLE_SANITIZERS CMake option to build the test programs with the address and UB sanitizers, and a CI job that uses it +### Changed + +- Updated actions versions (actions/checkout to v7) + ### Fixed - Removed the trace logging from `indexed_factory::remove_class()`, which caused a heap-use-after-free during static destruction: `remove_class()` is called from `~indexed_registrar()`, at which point the static logger it created may already have been destroyed