Mirrors/Catch2
0
0
Fork 0
mirror of https://github.com/catchorg/Catch2.git synced 2026-01-18 17:21:43 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix lifetime issues when using UNSCOPED_X message macros

Browse Source 
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 f80956a43a, 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.
This commit is contained in:
Martin Hořeňovský
2025-12-26 15:31:20 +01:00
parent 343cc059fe
commit eb3811c555
3 changed files with 111 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

119
src/catch2/internal/catch_run_context.cpp
View File

@@ -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<MessageInfo> messages;
// IDs of messages from UNSCOPED_X macros, which we have to
// remove manually.
std::vector<unsigned int> 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<MessageInfo> 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<MessageInfo>& g_messages() {
static CATCH_INTERNAL_THREAD_LOCAL std::vector<MessageInfo> value;
return value;
}
// Owners for the UNSCOPED_X information macro
static std::vector<ScopedMessage>& g_messageScopes() {
static CATCH_INTERNAL_THREAD_LOCAL std::vector<ScopedMessage> 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) {

15
tests/ExtraTests/CMakeLists.txt
View File

@@ -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
)

21
tests/ExtraTests/X94-ThreadSafetyTests.cpp
View File

@@ -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<std::thread> 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 );
}