From eb3811c555eccd2de8b63ba16e0f62c444970acf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Fri, 26 Dec 2025 15:31:20 +0100 Subject: [PATCH] Fix lifetime issues when using UNSCOPED_X message macros The original implementation of `UNSCOPED_X` message macros used a clever hack to make the original implementation simpler: construct an instance of `ScopedMessage` to manage its lifetime, but store it in a vector, so its lifetime is not actually scope-based, and we can manage it through the vector instance. This hack made it so that the lifetime of the vector that manages the fake `ScopedMessage`s must be outlived by the vector with the actual messages. Originally this wasn't a problem, because they both lived inside the run context instance. However, since then these vectors became globals and thread-local. When this happened, it still wasn't a problem; the two globals were declared in the right order, so they were destroyed in the right order as well. Then, in f80956a43a3e276686ed59531b41617282d75a0c, these globals were turned into magic static globals to improve their behaviour in MSVC's Debug build mode. This caused their lifetimes to be runtime-dependent; if a specific test thread added its first scoped message before it added first unscoped message, the lifetimes would be correct. If it instead added first unscoped message before adding first scoped message, then there **might** be invalid reads during thread destruction. The fix is simple: do things properly and manage the lifetime of messages in `UNSCOPED_X` explicitly. Then we don't have to deal with the destruction of fake `ScopedMessage`s while the thread is being destroyed, and the lifetime of the two vectors is no longer tied together. I also threw them both into a new type, to encapsulate some of the unscoped message logic. --- src/catch2/internal/catch_run_context.cpp | 119 ++++++++++++++------- tests/ExtraTests/CMakeLists.txt | 15 ++- tests/ExtraTests/X94-ThreadSafetyTests.cpp | 21 ++++ 3 files changed, 111 insertions(+), 44 deletions(-) diff --git a/src/catch2/internal/catch_run_context.cpp b/src/catch2/internal/catch_run_context.cpp index 1321d822..d63f19ac 100644 --- a/src/catch2/internal/catch_run_context.cpp +++ b/src/catch2/internal/catch_run_context.cpp @@ -195,17 +195,75 @@ namespace Catch { // clear there for performance reasons. static CATCH_INTERNAL_THREAD_LOCAL bool g_clearMessageScopes = false; + + // Holds the data for both scoped and unscoped messages together, + // to avoid issues where their lifetimes start in wrong order, + // and then are destroyed in wrong order. + class MessageHolder { + // The actual message vector passed to the reporters + std::vector messages; + // IDs of messages from UNSCOPED_X macros, which we have to + // remove manually. + std::vector unscoped_ids; + + public: + // We do not need to special-case the unscoped messages when + // we only keep around the raw msg ids. + ~MessageHolder() = default; + + + void addUnscopedMessage(MessageBuilder&& builder) { + repairUnscopedMessageInvariant(); + MessageInfo info( CATCH_MOVE( builder.m_info ) ); + info.message = builder.m_stream.str(); + unscoped_ids.push_back( info.sequence ); + messages.push_back( CATCH_MOVE( info ) ); + } + + void addScopedMessage(MessageInfo&& info) { + messages.push_back( CATCH_MOVE( info ) ); + } + + std::vector const& getMessages() const { + return messages; + } + + void removeMessage( unsigned int messageId ) { + // Note: On average, it would probably be better to look for + // the message backwards. However, we do not expect to have + // to deal with more messages than low single digits, so + // the improvement is tiny, and we would have to hand-write + // the loop to avoid terrible codegen of reverse iterators + // in debug mode. + auto iter = + std::find_if( messages.begin(), + messages.end(), + [messageId]( MessageInfo const& msg ) { + return msg.sequence == messageId; + } ); + assert( iter != messages.end() && + "Trying to remove non-existent message." ); + messages.erase( iter ); + } + + void removeUnscopedMessages() { + for ( const auto messageId : unscoped_ids ) { + removeMessage( messageId ); + } + unscoped_ids.clear(); + g_clearMessageScopes = false; + } + + void repairUnscopedMessageInvariant() { + if ( g_clearMessageScopes ) { removeUnscopedMessages(); } + g_clearMessageScopes = false; + } + }; + CATCH_INTERNAL_START_WARNINGS_SUPPRESSION CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS - // Actual messages to be provided to the reporter - static std::vector& g_messages() { - static CATCH_INTERNAL_THREAD_LOCAL std::vector value; - return value; - } - - // Owners for the UNSCOPED_X information macro - static std::vector& g_messageScopes() { - static CATCH_INTERNAL_THREAD_LOCAL std::vector value; + static MessageHolder& g_messageHolder() { + static CATCH_INTERNAL_THREAD_LOCAL MessageHolder value; return value; } CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION @@ -353,21 +411,19 @@ namespace Catch { Detail::g_lastAssertionPassed = true; } - if ( Detail::g_clearMessageScopes ) { - Detail::g_messageScopes().clear(); - Detail::g_clearMessageScopes = false; - } + auto& msgHolder = Detail::g_messageHolder(); + msgHolder.repairUnscopedMessageInvariant(); // From here, we are touching shared state and need mutex. Detail::LockGuard lock( m_assertionMutex ); { auto _ = scopedDeactivate( *m_outputRedirect ); updateTotalsFromAtomics(); - m_reporter->assertionEnded( AssertionStats( result, Detail::g_messages(), m_totals ) ); + m_reporter->assertionEnded( AssertionStats( result, msgHolder.getMessages(), m_totals ) ); } if ( result.getResultType() != ResultWas::Warning ) { - Detail::g_messageScopes().clear(); + msgHolder.removeUnscopedMessages(); } // Reset working state. assertion info will be reset after @@ -656,10 +712,10 @@ namespace Catch { m_testCaseTracker->close(); handleUnfinishedSections(); - Detail::g_messageScopes().clear(); - // TBD: At this point, m_messages should be empty. Do we want to - // assert that this is true, or keep the defensive clear call? - Detail::g_messages().clear(); + auto& msgHolder = Detail::g_messageHolder(); + msgHolder.removeUnscopedMessages(); + assert( msgHolder.getMessages().empty() && + "There should be no leftover messages after the test ends" ); SectionStats testCaseSectionStats(CATCH_MOVE(testCaseSection), assertions, duration, missingAssertions); m_reporter->sectionEnded(testCaseSectionStats); @@ -827,34 +883,15 @@ namespace Catch { } void IResultCapture::pushScopedMessage( MessageInfo&& message ) { - Detail::g_messages().push_back( CATCH_MOVE( message ) ); + Detail::g_messageHolder().addScopedMessage( CATCH_MOVE( message ) ); } void IResultCapture::popScopedMessage( unsigned int messageId ) { - // Note: On average, it would probably be better to look for the message - // backwards. However, we do not expect to have to deal with more - // messages than low single digits, so the optimization is tiny, - // and we would have to hand-write the loop to avoid terrible - // codegen of reverse iterators in debug mode. - auto& messages = Detail::g_messages(); - messages.erase( std::find_if( messages.begin(), - messages.end(), - [=]( MessageInfo const& msg ) { - return msg.sequence == - messageId; - } ) ); + Detail::g_messageHolder().removeMessage( messageId ); } void IResultCapture::emplaceUnscopedMessage( MessageBuilder&& builder ) { - // Invalid unscoped messages are lazy cleared. If we have any, - // we have to get rid of them before adding new ones, or the - // delayed clear in assertion handling will erase the valid ones - // as well. - if ( Detail::g_clearMessageScopes ) { - Detail::g_messageScopes().clear(); - Detail::g_clearMessageScopes = false; - } - Detail::g_messageScopes().emplace_back( CATCH_MOVE( builder ) ); + Detail::g_messageHolder().addUnscopedMessage( CATCH_MOVE( builder ) ); } void seedRng(IConfig const& config) { diff --git a/tests/ExtraTests/CMakeLists.txt b/tests/ExtraTests/CMakeLists.txt index 94eab4f0..60993b13 100644 --- a/tests/ExtraTests/CMakeLists.txt +++ b/tests/ExtraTests/CMakeLists.txt @@ -572,11 +572,20 @@ add_executable(ThreadSafetyTests ) target_link_libraries(ThreadSafetyTests Catch2_buildall_interface) target_compile_definitions(ThreadSafetyTests PUBLIC CATCH_CONFIG_EXPERIMENTAL_THREAD_SAFE_ASSERTIONS) -add_test(NAME ThreadSafetyTests + +add_test(NAME ThreadSafetyTests::ScopedMessagesAndAssertions COMMAND ThreadSafetyTests -r compact "Failed REQUIRE in the main thread is fine" ) -set_tests_properties(ThreadSafetyTests +set_tests_properties(ThreadSafetyTests::ScopedMessagesAndAssertions PROPERTIES - PASS_REGULAR_EXPRESSION "assertions: 801 | 400 passed | 401 failed" + PASS_REGULAR_EXPRESSION "assertions: 801 \\| 400 passed \\| 401 failed as expected" + RUN_SERIAL ON +) +add_test(NAME ThreadSafetyTests::UnscopedMessagesAndAssertions + COMMAND ThreadSafetyTests -r compact "Using unscoped messages in sibling threads" +) +set_tests_properties(ThreadSafetyTests::UnscopedMessagesAndAssertions + PROPERTIES + PASS_REGULAR_EXPRESSION "assertions: 401 \\| 401 failed as expected" RUN_SERIAL ON ) diff --git a/tests/ExtraTests/X94-ThreadSafetyTests.cpp b/tests/ExtraTests/X94-ThreadSafetyTests.cpp index d391417b..3f8ba9fa 100644 --- a/tests/ExtraTests/X94-ThreadSafetyTests.cpp +++ b/tests/ExtraTests/X94-ThreadSafetyTests.cpp @@ -41,3 +41,24 @@ TEST_CASE( "Failed REQUIRE in the main thread is fine", "[!shouldfail]" ) { REQUIRE( false ); } + +TEST_CASE( "Using unscoped messages in sibling threads", "[!shouldfail]" ) { + std::vector threads; + for ( size_t t = 0; t < 4; ++t) { + threads.emplace_back( [t]() { + UNSCOPED_INFO("thread " << t << " start"); + for (size_t i = 0; i < 100; ++i) { + for (size_t j = 0; j < 4; ++j) { + UNSCOPED_INFO("t=" << i << ", " << j); + } + CHECK( false ); + } + } ); + } + + for (auto& t : threads) { + t.join(); + } + + REQUIRE( false ); +}