Skip to content

Fix SIGSEGV in AssembleHalfedges on broken face topology#1652

Open
DatanoiseTV wants to merge 1 commit into
elalish:masterfrom
DatanoiseTV:fix/face2tri-crash
Open

Fix SIGSEGV in AssembleHalfedges on broken face topology#1652
DatanoiseTV wants to merge 1 commit into
elalish:masterfrom
DatanoiseTV:fix/face2tri-crash

Conversation

@DatanoiseTV
Copy link
Copy Markdown

Summary

Fixes a crash (SIGSEGV / EXC_BAD_ACCESS) in AssembleHalfedges (face_op.cpp) that occurs during Face2Tri in release builds when boolean operations produce degenerate faces with broken halfedge topology.

  • Root cause: vert_edge.find() returns end() when a face's halfedge chain is broken (endVert has no matching startVert). The only guard was DEBUG_ASSERT, which is a no-op in release builds. Dereferencing end() is UB — it produces a garbage edge index causing an out-of-bounds read on the halfedge array.
  • Reproduction: Triggered by complex CSG unions (BatchBooleanBoolean3::ResultFace2Tri) with high-polygon meshes under TBB parallelism. Reproducible on macOS with Apple Silicon (M5 Pro) but the bug is platform-independent.
  • Crash signature: SIGSEGV in (anonymous namespace)::AssembleHalfedges at various offsets (+108, +184, +224), always accessing an address just past the end of a MALLOC_LARGE region.

Changes

Three layers of defense in face_op.cpp:

  1. AssembleHalfedges: Added bounds check on thisEdge before dereferencing, and replaced the DEBUG_ASSERT-only check with a hard check that breaks out of the loop gracefully (drains remaining edges)
  2. Quad path (4-edge faces): Verify AssembleHalfedges returned at least 4 edges before indexing quad[0..3]; fall through to general triangulation if not
  3. Triangle path (3-edge faces): Skip degenerate triangles where edges don't form a valid closed loop

The fix is conservative — it prevents the crash and skips degenerate faces rather than producing corrupt geometry. The underlying cause (boolean operations producing faces with broken topology) may warrant a separate investigation.

Test plan

  • Headless render of 201 high-polygon sphere unions (3.3M facets) completes without crash
  • GUI render of complex CSG models no longer crashes on F6
  • Upstream test suite passes (CI will confirm)

AssembleHalfedges uses a multimap lookup to follow halfedge chains.
When the topology is broken (endVert has no matching startVert among
remaining edges), the find() returns end(). The existing DEBUG_ASSERT
only fires in debug builds — in release builds, dereferencing end()
is UB, producing a garbage edge index that causes an out-of-bounds
read on the halfedge array, leading to SIGSEGV.

This is reproducible with complex boolean operations (BatchBoolean
via Face2Tri) where degenerate faces can be produced.

Fix:
- Replace DEBUG_ASSERT-only check with a hard check that breaks out
  of the loop gracefully, draining remaining edges
- Add bounds check on thisEdge before dereferencing
- Protect the quad path: if AssembleHalfedges returns fewer than 4
  edges, fall through to general triangulation instead of crashing
- Protect the triangle path: skip degenerate triangles where edges
  don't form a valid loop
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 13, 2026

Codecov Report

❌ Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.27%. Comparing base (cf01c7b) to head (f837773).
⚠️ Report is 41 commits behind head on master.

Files with missing lines Patch % Lines
src/face_op.cpp 78.57% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1652      +/-   ##
==========================================
+ Coverage   92.59%   95.27%   +2.67%     
==========================================
  Files          33       34       +1     
  Lines        6048     7804    +1756     
==========================================
+ Hits         5600     7435    +1835     
+ Misses        448      369      -79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pca006132
Copy link
Copy Markdown
Collaborator

Can you give a test case that triggers the bad topology? I think we want to fix that instead of just avoiding segfault.

@DatanoiseTV
Copy link
Copy Markdown
Author

Fair enough — without a standalone reproducer this is hard to act on. I only hit it through OpenSCAD's GUI render path and couldn't strip it down to a manifold-level test case. The headless path with the same geometry didn't crash reliably.

Happy to close this. If someone does hit the same crash (SIGSEGV in AssembleHalfedges, release build, always at the vert_edge.find()end() dereference), the fix is straightforward — just guard the find result before dereferencing.

@DatanoiseTV
Copy link
Copy Markdown
Author

Tried to build a standalone reproducer — ran BatchBoolean with 200 random overlapping spheres at varying $fn, 100 different random seeds. Couldn't trigger it.

The crash only happened through OpenSCAD's GUI render path (CGALWorker thread), so it might be related to how OpenSCAD constructs the input meshes before passing them to manifold, or some specific near-degenerate geometry that the random tests don't generate.

Since I can't provide a test case, probably best to close this. The defensive check is still useful as a safety net in release builds (the existing DEBUG_ASSERT is a no-op in release), but I understand wanting to fix the root cause instead.

@pca006132
Copy link
Copy Markdown
Collaborator

A reproducer with openscad is still good, at least we can look into what happened.

@DatanoiseTV
Copy link
Copy Markdown
Author

Makes sense — will put together a .scad + build invocation that reproduces it and post back here.

@elalish
Copy link
Copy Markdown
Owner

elalish commented Apr 28, 2026

@DatanoiseTV any luck, or should we close this as no repro?

@DatanoiseTV
Copy link
Copy Markdown
Author

@DatanoiseTV any luck, or should we close this as no repro?

Unfortunatenly I am currently occupied with other projects. Will look in 2 weeks.

@pca006132
Copy link
Copy Markdown
Collaborator

@DatanoiseTV any update? Also, we updated the code a bit, so maybe you can check again to see if there is any segfault.

@DatanoiseTV
Copy link
Copy Markdown
Author

@DatanoiseTV any update? Also, we updated the code a bit, so maybe you can check again to see if there is any segfault.

Currently busy with other projects, but it is on my todo list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants