[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 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
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. |