[SERVER-53085] Undefined behavior in ALL C++ unit test ASSERT statements (yes all) Created: 25/Nov/20  Updated: 27/Oct/23  Resolved: 28/Nov/20

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Billy Donahue Assignee: Backlog - Service Architecture
Resolution: Works as Designed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Service Arch
Participants:

 Description   

https://en.cppreference.com/w/cpp/memory/unique_ptr/~unique_ptr
unique_ptr's dtor requires that the deleter must not throw. And yet, we're trying to use a unique_ptr to trigger a throw in unit tests, via the destructor ~TestAssertionFailure() noexcept(false).

Well it doesn't matter if the object is marked noexcept(false) or not. An object with a throwing destructor CANNOT be destroyed by a unique_ptr going out of scope. It's UB.

I think this means that any ASSERT failure in our test framework is UB.

https://github.com/mongodb/mongo/blob/master/src/mongo/unittest/unittest.cpp#L598

We can fix by just using something other than std::unique_ptr that doesn't have this restriction and is friendly to exception propagation out of destructors.



 Comments   
Comment by Billy Donahue [ 28/Nov/20 ]

False alarm! This is very subtle.

A TestAssertionFailure is marked "enabled" when its stream() member is called.

A TestAssertionFailure that's marked "enabled" throws a TestAssertionFailureException upon destruction.

A TestAssertionFailure is held as a unique_ptr member of ComparisonAssertion. This unique_ptr is nonempty for failed {{ASSERT}}s.

At the end of the failed ASSERT call, the ComparisonAssertion is destroyed, the unique_ptr with it.

The ASSERT call expands like this:

https://github.com/mongodb/mongo/blob/master/src/mongo/unittest/unittest.h#L113

/**
 * Binary comparison utility macro. Do not use directly.
 */
#define ASSERT_COMPARISON_(OP, a, b) ASSERT_COMPARISON_STR_(OP, a, b, #a, #b)
 
#define ASSERT_COMPARISON_STR_(OP, a, b, aExpr, bExpr)                                         \
    if (auto ca =                                                                              \
            ::mongo::unittest::ComparisonAssertion<::mongo::unittest::ComparisonOp::OP>::make( \
                __FILE__, __LINE__, aExpr, bExpr, a, b);                                       \
        !ca) {                                                                                 \
    } else                                                                                     \
        ca.failure().stream()

But here's the trick. The failure() member returns a TestAssertionFailure by value. This makes a COPY of the pointee of the unique_ptr, and it is this copy, not the original TestAssertionFailure, on which stream() is called, marking it enabled. The TestAssertionFailure held by unique_ptr inside the ComparisonAssertion is never marked enabled, and doesn't throw from the unique_ptr destructor. Only the copy does this.

This can all be SIGNIFICANTLY simplified, but there's nothing to worry about in terms of correctness.

Generated at Thu Feb 08 05:29:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.