Placing a guard for undefined behaviour that appears to trigger sanit… #235
Conversation
Reviewer's Guide by SourceryThis pull request introduces a guard against undefined behavior in the Sequence diagram for Catch2ApprovalListener section handling with guardsequenceDiagram
participant Test as Test Case
participant Listener as Catch2ApprovalListener
participant Sections as sections vector
Test->>Listener: sectionEnded()
Listener->>Sections: check if empty()
alt sections is not empty
Listener->>Sections: pop_back()
else sections is empty
Note over Listener: Skip pop_back()
Note over Listener: Prevent undefined behavior
end
State diagram for sections vector safety checkstateDiagram-v2
[*] --> CheckSections
CheckSections --> Empty: sections.empty()
CheckSections --> NotEmpty: !sections.empty()
Empty --> [*]: Skip pop_back
NotEmpty --> PopSection: Safe to pop
PopSection --> [*]: sections.pop_back()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
We did eventually discover a problem with our assert() implementation on release builds (some gcc optimization managed to break the weird legacy implementation we used), witch let to null pointers and signals/ threads that die unpredictably. I am not exactly sure why our stack of Catch2, Approval Tests, and the GitLab continuous integration pipelines ended up reporting this later undefined behavior in Catch2 instead of reporting the stack overflow in the software itself. (and why the execution of the test continued in spite of the signal). But now it feel like things could have gone completely unnoticed if not for this error. As for replicating this, it's hard to identify the exact conditions needed. I would start by creating a test program with a thread that ends by stack overflow. Then that signal should be suppressed in such a way that allows the test software to continue running, thus arriving at this situation when it's trying to pop a section that no longer exists. Regardless, it's all very circumstantial. I might try my hand at this, but I will see. And thanks for the advice as well, it was very helpful in getting the right mindset to find this bug. |
Description
For the last few weeks we have been getting weird sanitizer hits for this exact function inside our automated testing pipeline. It seems to depend a lot on the order of the sections, if those sections have exceptions handled within them, etc, etc. It is incredibly hard to replicate and debug this anomaly. I am not even able to replicate it outside the GitLab pipeline itself. I noticed this unguarded undefined behavior and it's pretty hard for me not to blame this. So I hope you accept this change.
The solution
Added a guard against undefined behavior.
No tests or behavior should be affected.
The report in question always looks like this:
Summary by Sourcery
Bug Fixes:
Summary by Sourcery
Bug Fixes:
Catch2ApprovalListener::sectionEndedwith an empty sections vector.