Repository navigation
Conversation
…c-static lock BRepLib::Plane() created the process-wide plane on first use with no lock, so concurrent first use of BRepLib_MakeEdge2d read a plane the losing thread had released. Hold it in a function-local static, as Message::DefaultMessenger() does, and keep the setter's behaviour that a null argument restores the default. Add BRepLib_Test.Plane_ConcurrentFirstUse and Plane_NullRestoresDefault.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BRepLib::Plane() creates its process-wide plane on first use:
static occ::handle<Geom_Plane> thePlane;
...
if (thePlane.IsNull()) { thePlane = new Geom_Plane(gp::XOY()); }
return thePlane;
BRepLib_MakeEdge2d reads every vertex it builds through that plane (Point, Project, UpdateEdge). Two threads making their first 2D edge together both see a null handle and both assign. The losing assignment releases the plane the other thread has already read through, so a vertex is read from freed memory: zero or denormal coordinates, or a crash (SIGSEGV, SIGBUS, SIGTRAP, abort in free).
The plane is now created by a function-local static, which C++11 initialises under a lock, as Message::DefaultMessenger() creates its messenger. The setter keeps its behaviour, including that a null argument restores the default plane. The setter itself is still unsynchronised, like BRepLib::Precision's.
Plane_ConcurrentFirstUse clears the plane with BRepLib::Plane(null) to re-arm the first use, releases 16 threads together on a barrier, and checks that every thread was handed the same plane; 500 rounds, in a death-test child process because the unsynchronised race can corrupt the heap. It compares the plane pointers and never dereferences one.
Measured on a local macOS arm64 Release build (clang, -Werror -Wall -Wextra, BUILD_GTEST=ON, Foundation, Modeling Data and Modeling Algorithms modules) of IR at 0089343, on a heavily loaded machine: