Modeling Algorithms - guard CreateSmoothed's fixed-stride shapes array against a section with more edges than section 1 - #1466
Open
gsdali wants to merge 2 commits into
Conversation
…y against a section with more edges than section 1 BRepOffsetAPI_ThruSections::CreateSmoothed() derives nbEdges from section 1 alone and allocates shapes sized nbSects * nbEdges, then fills it walking each section's actual edges with no bounds check. With CheckCompatibility(false), nothing reconciles differing section edge counts first, so a later section with more edges than section 1 walks past the end of shapes: an out-of-bounds write, observed as heap corruption and a SIGSEGV/SIGBUS once at least 3 sections are involved (2 sections always take the CreateRuled() path instead). Fix: count each non-punctual section's edges before allocating shapes; on a mismatch, set myStatus to ProfilesInconsistent and return, matching the early-return idiom this function already uses two lines below. Adds two GTests to the existing BRepOffsetAPI_ThruSections_Test.cxx: the crash reproducer (now fails cleanly instead of crashing) and a regression guard for matching edge counts, including a punctual end section.
…xemption, DRY the predicate The original fix's inequality test already rejected a section with MORE edges than section 1 (the crashing direction) or FEWER (previously silent success with misaligned per-section strides -- same contract violation, just without a crash to signal it). Added a GTest proving the fewer-edges direction: fails against pristine, unpatched source (IsDone() == true, Shape().IsValid() == false), passes after the fix. The punctual-section exemption now also verifies the section actually has at least one edge before exempting it, rather than trusting w1Point/w2Point's classification unconditionally -- those are vacuously true for a wire with zero edges, which the fill loop's punctual branch would otherwise walk with an uninitialized WireExplorer. Attempted to reproduce a live crash for this specific case (including via a reused builder) and could not; the check is kept as a correct, cheap hardening, not as a proven fix for an observed crash. isPunctualSection is now a single local lambda shared by this guard and the pre-existing fill loop 15 lines below, which used to duplicate the same two-clause boolean expression separately.
gsdali
force-pushed
the
fix/thrusections-createsmoothed-section-edge-count
branch
from
September 2, 2026 00:30
2dbc7c4 to
94e1444
Compare
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.
Description
BRepOffsetAPI_ThruSections::CreateSmoothed()derivesnbEdges— the edge count it assumesevery section has — from section 1 alone (or section 2, if section 1 is a punctual/degenerate
vertex section). It then allocates
shapes, anNCollection_Array1<TopoDS_Shape>sized exactlynbSects * nbEdges, and fills it by walking each section's wire with aBRepTools_WireExplorer,incrementing a running index with no bounds check.
Nothing enforces that every section actually has
nbEdgesedges.BRepFill_CompatibleWires(viaCheckCompatibility(true), the default) normally reconciles differing edge counts across sectionsbefore
CreateSmoothed()ever runs. WithCheckCompatibility(false)("no check"), thatreconciliation is skipped entirely, so a later section with more edges than section 1 makes the
fill loop walk past the end of
shapes— an out-of-bounds write, observed as heap corruption and aSIGSEGV/SIGBUS inside
CreateSmoothed()once at least 3 sections are involved (2 sections alwaystake the
CreateRuled()path instead, which does not share this allocation shape).Confirmed via a minimal, from-scratch C++ reproducer with a custom signal handler
(
backtrace_symbols_fd) for attribution: two matching circle sections build successfully; reusingthe same builder with a third, differently-shaped section (more edges than the first two) and
rebuilding SIGSEGVs against the stock library. It does not need a reused builder as such — what
actually varies is process/allocator state at the time of the overrun, so an otherwise-identical
single
Build()call with all three sections added up front does not reliably crash even thoughthe same out-of-bounds write still occurs. Verified directly by instrumenting the fill loop: the
write index reaches one past
shapes.Upper()before the corrupting assignment.Fix
Before allocating
shapes, walk every non-punctual section and count its edges; if any section'scount differs from
nbEdges, setmyStatustoBRepFill_ThruSectionErrorStatus_ProfilesInconsistentand return, matching the early-return idiom this function already uses two lines below
(
TS.IsNull()->myStatus = Failed; return;). Punctual end sections (a degenerate vertex addedvia
AddVertex(), e.g. a cone's apex) are exempt, matching the existingw1Point/w2Pointhandling throughout the rest of the function.
Testing
Two GTests added to the existing
BRepOffsetAPI_ThruSections_Test.cxx:MismatchedSectionEdgeCountFailsCleanlyWithoutCheck— the crash reproducer. Proved it failsfirst: linked against the unpatched archive, this test crashes (SIGBUS). With the fix, it passes
(
IsDone() == false, no crash).MatchingSectionEdgeCountsStillSucceedWithoutCheck— regression guard: matching section edgecounts under
CheckCompatibility(false)still build, including a punctual end section.All 5 tests in the file (the 3 pre-existing plus these 2) pass with the fix. Validated by
override-linking the patched translation unit ahead of the stock archive, both with and without
No_Exception/NDEBUG(matching a Release build configuration), confirming the fix eliminates theout-of-bounds write itself rather than relying on
Standard_OutOfRange's range check beingcompiled in.
Formatted with this repo's own
.clang-format(--dry-run --Werrorclean on both changed files).