From 2ff177f5ba4bcc209360a01993f0bc7448f2e513 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Sat, 15 Aug 2026 21:37:35 +1000 Subject: [PATCH 1/3] fix(#913): guard CreateSmoothed's fixed-stride shapes array against a mismatched section edge count BRepOffsetAPI_ThruSections::CreateSmoothed() derives the edge count it assumes every section has from section 1 alone, allocates a shapes array sized on that assumption, 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 overruns the array -- heap corruption, observed as SIGSEGV/SIGBUS once at least 3 sections are involved. Found while hunting for a failure trigger during #910's review, filed separately. Isolated with a standalone C++ repro (no Swift/bridge) and a custom SIGSEGV/SIGBUS handler, root-caused by instrumenting the fill loop directly (write index reaches one past the array bound before the crashing archive's own write). Confirmed the "reused builder" framing from the original report is a symptom, not the cause: a single Build() call with all mismatched sections from the start doesn't reliably crash either, even though the same out-of-bounds write occurs -- what varies is process/allocator state at the time of the overrun. Carried as Scripts/patches/0027-*, filed upstream as https://github.com/Open-Cascade-SAS/OCCT/pull/1466 (open, mergeable, 2 new GTests, both crash- and regression-proven). Verified against a full local rebuild (Scripts/build-occt.sh, all 17 patches including this one) rather than override-link alone: full swift test, 5533/5533 passing. New test mismatchedSectionEdgeCountWithoutCheckFailsCleanly() (StressBuilderLifecycleTests.swift): proved it crashes the test process with signal 11 against the currently pinned (unfixed) kernel, then passes cleanly against the rebuilt kernel. Closes #913 Co-Authored-By: Claude Sonnet 5 --- ...moothed-section-edge-count-guard-913.patch | 87 +++++++++++++++++++ Scripts/patches/README.md | 57 ++++++++++++ .../StressBuilderLifecycleTests.swift | 23 +++++ 3 files changed, 167 insertions(+) create mode 100644 Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch diff --git a/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch b/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch new file mode 100644 index 000000000..0ffa66c26 --- /dev/null +++ b/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch @@ -0,0 +1,87 @@ +From: OCCTSwift ecosystem +Subject: [PATCH] Modeling Algorithms - guard CreateSmoothed's fixed-stride + shapes array against a section with more edges than section 1 + +BRepOffsetAPI_ThruSections::CreateSmoothed() derives `nbEdges` -- the edge +count it assumes every section has -- from section 1 alone (or section 2, if +section 1 is a punctual/degenerate vertex section). It then allocates +`shapes`, an NCollection_Array1, sized exactly +`nbSects * nbEdges`, and fills it by walking each section's wire with a +BRepTools_WireExplorer, incrementing a running index with no bounds check. + +Nothing enforces that every section actually has `nbEdges` edges. +BRepFill_CompatibleWires (via CheckCompatibility(true), the default) +normally reconciles differing edge counts across sections before +CreateSmoothed() ever runs. With CheckCompatibility(false) ("no check"), +that reconciliation 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 a later SIGSEGV inside +CreateSmoothed() itself once at least 3 sections are involved (2 sections +always take the CreateRuled() path instead, which does not share this +allocation shape). Confirmed via a minimal, from-scratch C++ reproducer with +no OCCTSwift/bridge involvement: two matching circle sections build +successfully, reusing the same builder with a third, differently-shaped +section (more edges than the first two) and rebuilding SIGSEGVs on the +stock library. Needs no reused builder as such -- what actually varies is +process/allocator state at the time of the overrun, which is why an +otherwise-identical single Build() call with all three sections added up +front does not reliably crash even though the same out-of-bounds write +still occurs. + +Fix: before allocating `shapes`, walk every non-punctual section and count +its edges; if any section's count differs from `nbEdges`, set myStatus to +BRepFill_ThruSectionErrorStatus_ProfilesInconsistent and return, matching +the early-return idiom this function already uses two lines below +(TS.IsNull() -> myStatus = Failed; return;). Punctual end sections (a +degenerate vertex added via AddVertex(), e.g. a cone's apex) are exempt, +matching the existing w1Point/w2Point handling throughout the rest of the +function -- a point section legitimately has a different edge count and is +not itself walked by the fill loop's "punctual" branch. + +Validated by override-linking the patched translation unit ahead of the +stock archive (both with and without No_Exception/NDEBUG, matching the +production Release configuration this project ships): the original 3 +scenarios that legitimately succeed today (checkCompatibility(false) with +genuinely matching section edge counts; a punctual section at either end +mixed with matching wire sections; checkCompatibility(true) with +mismatched sections, reconciled by BRepFill_CompatibleWires as before) are +all byte-for-byte unaffected, and the crashing scenario now fails cleanly +(IsDone() == false) instead of crashing. + +SecondMouseAU/OCCTSwift#913. +--- + .../TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx | 23 +++++++++++++++++++++ + 1 file changed, 23 insertions(+) + +--- a/src/ModelingAlgorithms/TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx ++++ b/src/ModelingAlgorithms/TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx +@@ -747,6 +747,29 @@ + } + } + ++ // #913: nbEdges above comes from section 1 (or 2, if punctual) alone. CheckCompatibility(false) ++ // skips BRepFill_CompatibleWires, so nothing else guarantees every other section has the same ++ // edge count -- and the fixed-stride fill loop below indexes `shapes` on that assumption with no ++ // bounds check, overrunning it (heap corruption, observed as a later SIGSEGV) for a section with ++ // more edges than section 1. Refuse cleanly instead of writing out of bounds. ++ for (int iSect = 1; iSect <= nbSects; iSect++) ++ { ++ if ((iSect == 1 && w1Point) || (iSect == nbSects && w2Point)) ++ { ++ continue; // a punctual end section legitimately has a different (zero) edge count ++ } ++ int aSectEdges = 0; ++ for (anExp.Init(TopoDS::Wire(myWires(iSect))); anExp.More(); anExp.Next()) ++ { ++ aSectEdges++; ++ } ++ if (aSectEdges != nbEdges) ++ { ++ myStatus = BRepFill_ThruSectionErrorStatus_ProfilesInconsistent; ++ return; ++ } ++ } ++ + // recover the shapes + bool uClosed = true; + NCollection_Array1 shapes(1, nbSects * nbEdges); diff --git a/Scripts/patches/README.md b/Scripts/patches/README.md index c5d622661..2fd709c72 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -765,6 +765,63 @@ baked in (`Package.swift`'s own census: "ALL FIFTEEN ARE VERIFIED PRESENT IN THE `0026` is carried on disk only — the first patch since `0022`-`0025` themselves were folded into that release to sit outside the pin. Watch for it at the next kernel re-pin. +## 0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch + +**Fixes the upstream OCCT defect behind [#913](https://github.com/SecondMouseAU/OCCTSwift/issues/913)** +— found incidentally while investigating #910 (a different, bridge-side #905-review finding): +`ThruSectionsBuilder`, given `checkCompatibility(false)` and a section whose edge count differs +from the first section, SIGSEGVs instead of failing cleanly. + +`CreateSmoothed()` derives `nbEdges` — the edge count it assumes every section has — from section 1 +alone (or section 2, if section 1 is a punctual/degenerate vertex section). It allocates `shapes`, +an `NCollection_Array1` sized exactly `nbSects * nbEdges`, then fills it by walking +each section's wire with a `BRepTools_WireExplorer`, incrementing a running index with **no bounds +check**. Nothing enforces that every section actually has `nbEdges` edges: `BRepFill_CompatibleWires` +(via `checkCompatibility(true)`, the default) normally reconciles differing edge counts across +sections before `CreateSmoothed()` ever runs, but `checkCompatibility(false)` skips that entirely. +A later section with **more** edges than section 1 walks the fill loop straight past the end of +`shapes` — an out-of-bounds write, corrupting adjacent heap memory rather than raising a catchable +failure. + +**Reached only with 3+ sections.** With exactly 2 sections, `Build()` always takes the +`CreateRuled()` path instead (`if (myWires.Length() == 2 || myIsRuled) CreateRuled(); else +CreateSmoothed();`), which builds its shell via `BRepFill_Generator` — a different mechanism that +doesn't share this fixed-stride allocation. A single `Build()` call with all mismatched sections +present from the start does **not** reliably crash even though the identical out-of-bounds write +still occurs; what actually varies is process/allocator state at the time of the overrun (confirmed +directly: instrumenting the fill loop shows the write index reaching one past `shapes.Upper()` +either way, but a from-scratch process quietly lands the write in unmapped-but-harmless heap slack +where a process that already ran one successful `Build()` call does not). Symptom, not cause: this +made the defect look like it needed a *reused* builder when it doesn't — any 3+-section +`checkCompatibility(false)` call with mismatched edge counts carries the same latent corruption. + +**Fix:** before allocating `shapes`, walk every non-punctual section and count its edges; on a +mismatch, set `myStatus` to `BRepFill_ThruSectionErrorStatus_ProfilesInconsistent` and return, +matching the early-return idiom this function already uses two lines below (`TS.IsNull()` -> +`myStatus = Failed; return;`). Punctual end sections (`AddVertex()`, e.g. a cone's apex) are exempt, +matching the existing `w1Point`/`w2Point` handling throughout the rest of the function — a point +section legitimately has a different (zero) edge count and the fill loop's punctual branch doesn't +walk it the same way. + +**Validation** (override-link, no full rebuild for this patch's own writeup — see `#0001`'s retired +entry for the technique; the OCCTSwift-side PR carrying this one also rebuilds the local +xcframework, since #913 asked for that explicitly rather than deferring it like `0026`): compiled +and linked both the unpatched and patched `.cxx` ahead of the pinned `libOCCT-macos.a`. Against the +unpatched override, a new GTest (`BRepOffsetAPI_ThruSections_Test.cxx`, +`MismatchedSectionEdgeCountFailsCleanlyWithoutCheck`) crashes (SIGBUS) exactly as the issue +describes; against the patched override it passes (`IsDone() == false`, no crash), along with a +second new regression-guard test (`MatchingSectionEdgeCountsStillSucceedWithoutCheck`, matching +edge counts including a punctual end section) and all three pre-existing tests in the file — 5/5. +Re-confirmed the fix holds with `No_Exception`/`NDEBUG` defined (matching a Release build +configuration close to what the shipped archive itself was likely built with): the fix eliminates +the out-of-bounds write itself, so it does not depend on `Standard_OutOfRange`'s range check being +compiled in. `clang-format --dry-run --Werror` against OCCT's own `.clang-format` reports zero +violations on both changed files. + +Filed upstream as [OCCT#1466](https://github.com/Open-Cascade-SAS/OCCT/pull/1466). + +**Retire** once the bundled OCCT includes this fix. + # Retired patches The `.patch` files below are **deleted**. Each fix now comes from the pinned OCCT release itself, so diff --git a/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift b/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift index c58fec572..6c6045592 100644 --- a/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift +++ b/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift @@ -430,6 +430,29 @@ struct StressThruSectionsBuilderLifecycleTests { _ = loft.build() _ = loft.build() } + + // #913: checkCompatibility(false) skips BRepFill_CompatibleWires' section reconciliation, so + // nothing else guarantees every section has the same edge count. CreateSmoothed()'s fill loop + // (reached only at 3+ sections — 2 sections always take the CreateRuled() path instead) walked + // a fixed-stride array sized from section 1 alone with no bounds check, overrunning it and + // SIGSEGVing for a later section with more edges than the first. Must fail cleanly instead. + @Test func mismatchedSectionEdgeCountWithoutCheckFailsCleanly() { + guard let w1 = Wire.circle(origin: SIMD3(0, 0, 0), normal: SIMD3(0, 0, 1), radius: 5), + let w2 = Wire.circle(origin: SIMD3(0, 0, 10), normal: SIMD3(0, 0, 1), radius: 3), + let s1 = Shape.fromWire(w1), let s2 = Shape.fromWire(w2) else { return } + let loft = ThruSectionsBuilder(isSolid: true, isRuled: false) + loft.checkCompatibility(false) + loft.addWire(s1) + loft.addWire(s2) + #expect(loft.build()) + + // A third section with MORE edges (a triangle, 3) than the first two (1 each, circles). + guard let triangle = Wire.polygon3D([ + SIMD3(2, 0, 20), SIMD3(-1, 1.7320508, 20), SIMD3(-1, -1.7320508, 20) + ], closed: true), let triangleShape = Shape.fromWire(triangle) else { return } + loft.addWire(triangleShape) + #expect(!loft.build()) + } } // MARK: - CellsBuilder From 702ff2ccd4cf3dd8c63e92ac157d12c9bd8c265e Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Sun, 16 Aug 2026 01:04:05 +1000 Subject: [PATCH 2/3] fix(#913): address PR #915 review findings 12 findings, all addressed or explicitly documented. Finding 1 (critical, confirmed live via this PR's own first CI run, which crashed swift build + test (macOS) with exactly the predicted SIGSEGV): mismatchedSectionEdgeCountWithoutCheckFailsCleanly needs patch 0027, which isn't in Package.swift's pinned kernel asset. SwiftPM runs every test target in one process, so an unguarded crash aborts the whole suite for every future PR until the pin moves -- the #585 failure shape. Gated on @Test(.enabled(if: ProcessInfo.processInfo.environment["OCCTSWIFT_LOCAL"] == "1")), matching #905/PR #909's precedent of adding no Swift test at all for the identical reason. kernel-integration.yml sets OCCTSWIFT_LOCAL=1 when it builds Scripts/patches/ from source, so the test still runs (and is verified) there. Finding 3 (real, verified empirically before documenting): the guard's inequality test already rejected fewer-edge sections, not just more -- previously silent, invalid-but-"successful" results, not just crashes. Documented accurately everywhere (patch header, README, CLAUDE.md, this PR's own SemVer note, which previously undersold this direction). Finding 4: the punctual-section exemption now also verifies a section has at least one edge before exempting it (w1Point/w2Point are vacuously true for a genuinely empty wire). Attempted to reproduce a live crash for this via both a fresh and reused builder and could not -- documented as a hardening, not a proven fix; the GTest written for it could not be made to fail without the check and was removed rather than kept as unproven coverage. Finding 5: added punctualApexWithMatchingSectionsStillSucceedsUnderCreateSmoothed -- nothing previously pinned a legitimate cone-apex loft succeeding once #913's guard reaches CreateSmoothed (3+ sections). Doesn't depend on patch 0027 (the exemption itself is unmodified pre-existing behavior), so it isn't gated; verified passing against both the pinned and the patched kernel. Finding 6: added a CLAUDE.md Known OCCT Bugs entry, matching every other carried patch. Finding 7: committed a reproducer at Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/, matching #905's own convention (README + standalone .mm + stock.txt/patched.txt). Finding 8: converted guard-let-else-return setup to try #require(...). Finding 9: hoisted the punctual-section predicate into a lambda shared with the pre-existing fill loop, in both the local patch and the already-submitted upstream OCCT#1466 PR (new commit pushed there). Finding 11: fixed the factually wrong "(zero) edge count" comment -- AddVertex() creates a wire with exactly one degenerate edge, not zero. Finding 12: SIGSEGV and SIGBUS were both genuinely observed, in different binaries (a custom-handler standalone reproducer vs. a GTest binary with OS default signal handling) -- not a contradiction to resolve to one signal, exactly what heap corruption looks like. Documented both, with which binary showed which. Findings 2, 10: no action needed / documented rationale for leaving the check's placement as-is (see the patch's own updated writeup). Verified against a full local kernel rebuild with all 17 patches (patch 0027 v2 correctly recognized as "already applied", confirming the regenerated patch file matches): full swift test 5534/5534 passing, all 6 static gates clean. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 34 ++++ ...moothed-section-edge-count-guard-913.patch | 128 +++++++++--- Scripts/patches/README.md | 108 +++++++--- .../README.md | 119 +++++++++++ .../occt_913_test.mm | 186 ++++++++++++++++++ .../patched.txt | 9 + .../stock.txt | 9 + .../StressBuilderLifecycleTests.swift | 46 ++++- 8 files changed, 574 insertions(+), 65 deletions(-) create mode 100644 Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/README.md create mode 100644 Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/occt_913_test.mm create mode 100644 Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/patched.txt create mode 100644 Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/stock.txt diff --git a/CLAUDE.md b/CLAUDE.md index 61ca8376f..a4f43d5c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -307,6 +307,40 @@ suite into these targets (each `Tests/OCCTTests/`, declared in `Package. - `GeomFill_Sweep::BuildAll` overwrites the measured C1-conversion error with the requested tolerance: **#597, the kernel half** (the bridge half, #741/#751, closed separately, see below). `SError` is set to the real measured error at `GeomFill_Sweep.cxx:286` (`Approx.MaxErrorOnSurf()`), but when `ForceApproxC1` is set and the swept surface isn't already C1 in V, the class re-approximates through `GeomConvert_ApproxSurface(mySurface, theTol, ...)` (`theTol` a literal `1.e-4`) and, on `HasResult()` (documented as true even for a result "not NECESSARILY within the required tolerance"), finishes with `SError = theTol;` instead of reading `MaxError()`, which reports what the conversion actually achieved and sits two lines above, unread. `BRepFill_Sweep`/`BRepFill_PipeShell`/`BRepOffsetAPI_MakePipeShell::ErrorOnSurface()` all forward `SError` verbatim, so `SetForceApproxC1(true)`, a public, documented API, hands every caller a number describing the request, not the result. **Reaching the branch needs a spine whose tangent discontinuity sits inside one edge**, not at a vertex: `BRepFill_Sweep` splits at spine vertices, so a polyline spine never gets there. The fixture (from #572, pinned by `Issue572SweepApproxTests.swift`) is a single-edge degree-2 B-spline spine with an interior knot of multiplicity 2, swept with a circle profile. **Checked, not assumed, that `MaxError()` is the right quantity**: unlike `GeomPlate_MakeApprox::ApproxError()` (the #571 trap, which measures an *intermediate* object and broke 6/6 real tests when gated on), `GeomConvert_ApproxSurface`'s `Surf` argument here *is* `mySurface`, the exact surface being replaced, confirmed by reconstructing the identical call from outside the kernel and finding its output's degree/pole counts and measured deviations bit-identical to the real forced build's. Also confirmed the number **moves**: patch `0019` (#522) is what makes this possible, since before it every interior truncation error was structurally zero; measured `MaxError() = 2.54714` against the pinned kernel, matching #572's own independent measurement of the same fixture to the printed digit, 25000x the `0.0001` the stock code reports. **Fixed** (`Scripts/patches/0025-*`, override-link validated, not yet in a rebuilt xcframework): `SError = ConvertApprox.MaxError();`, one line; the four `CError` literal `0.` entries a few lines above are left untouched rather than fabricated (see #726). Validation confirms this is diagnostic-only: every geometry value the reproducer prints (degree/pole/knot counts, two independent geometric deviations, same-parameter and nearest-point) is byte-identical before and after, since `mySurface` is already `ConvertApprox.Surface()` two statements earlier; no bridge site gates on this number today (`PipeShellBuilder.errorOnSurface` is info-only, and `OCCTGeomFillSweep`'s own error gate from #741 never sets `ForceApproxC1` so never reaches this branch), so `swift test` is unaffected. See [`Scripts/repro/597-geomfill-sweep-error-overwrite/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-geomfill-sweep-error-overwrite) for the full writeup. Upstream PR drafted but not sent (`draft-pr.md` in that directory), per `okf/policies/upstream-occt-style.md`. #597. - **`GeomPlate_MakeApprox::ApproxError()` and `BRepOffsetAPI_MakeFilling::G0Error()` look like the obvious gate for "accepted an approximation without reading its error" (Cluster E, #668), and are not: investigated and rejected on measurement, twice, for two different reasons. This is the bridge half of #597** (the kernel half is immediately above). `occtPlateApproxSurface` (`OCCTBridge_Internal.h`, backs `OCCTShapePlatePoints`/`OCCTShapePlateCurves` + 4 more entry points) and `OCCTShapeFillBuildResult` (`OCCTBridge_Healing.mm`, backs `OCCTShapeFill`/`OCCTShapeFillWithSupport`/`OCCTShapeFillConstraints`) both run an approximation and never check the error it reports, the same shape #741 fixed for `OCCTGeomFillSweep`. Both candidate fixes (`if (approx.ApproxError() > tolerance) return null;` / `if (filling.G0Error() > tolerance) return null;`) were built and run against the real bridge, not just reasoned about, and both broke real, already-shipped, already-tested behaviour. `GeomPlate_MakeApprox::ApproxError()`'s own header doc gives away why the plate site is unfixable this way: it measures distance between the **fitted BSpline and the intermediate `GeomPlate_Surface`** the caller never sees, not fidelity to the caller's own input points. On the #571 fixture it exceeds `tolerance` by up to 5.8x while the deviation from the caller's own 25 input points stays inside tolerance throughout, and gating on it failed all 6 `Issue571PlateApproxTests`. `BRepOffsetAPI_MakeFilling::G0Error()` is the right kind of number for the fill site (`BRepFill_Filling::G0Error()`'s own header: "the maximum distance between the result and the constraints", the caller's own boundary, unlike the plate case) but is still unsafe to gate on: `FillingParameters`'s Swift default is `tolerance: Double = 1e-4`, not a fallback for an unset value, so `Tol3d = 1e-4` is the number **every** default `Shape.fill` call is built at, and legitimate, correct, higher-continuity or heavily-constrained fills routinely exceed it. Gating broke 2 of 17 `FillingSupportFaceTests` (`curvatureContinuityIsAccepted`, `internalConstraintIsNotABoundary`), both at the plain default tolerance, both asserting specific, correct, checked geometry. **A PR #751 review caught that the fill site's own supporting fixture initially understated this**: `wavyEdge()` in `occt_597_fill_g0_realistic.mm` built its sine-sampled boundary points as a single-span, full-multiplicity `Geom_BSplineCurve`, i.e. as control poles of a degree-13 Bezier, which damps a high-frequency control polygon severely (62.8% of the intended amplitude gone, measured directly from the Bernstein basis). Rebuilt on `GeomAPI_Interpolate` (which actually interpolates the sampled points, matching `OCCTCurve3DInterpolate`'s existing precedent in `OCCTBridge_Curve3D.mm`), the same boundary's `G0Error` jumps from "within the bridge's default tolerance" to exceeding even that loosest tested tolerance by ~2000x, and the accepted surface's control poles land up to ~530 units from a ~10-unit-scale boundary. `IsDone()` is still true, and `G0Error()` alone (0.2) does not communicate how far the fit has actually diverged. A draft of this entry then claimed no single `G0Error()` threshold could separate that diverged fit from the two regressing tests, without measuring what those two tests' own `G0Error()` actually were; measured directly (temporary debug prints in both `BRepOffsetAPI_MakeFilling::Build()` call sites, reverted after), they are 5.295e-4 (5.3x tolerance) and 1.231e-3 (12.3x tolerance), two to three orders of magnitude below the corrected fixture's ~2000x. So a fixed absolute threshold placed between those values and the diverged case (e.g. 0.01) would in fact separate them; the honest conclusion is narrower, not reversed: gating on the *caller's own requested tolerance* is unsafe and proven so by the two real regressions, while a *fixed* threshold is unjustified rather than impossible, since nobody has measured what it should be and picking one now would be inventing a number with as little basis as the `1e-4` this investigation already discredited, exactly the failure mode #726 exists to catch. No bridge fix shipped from this investigation; two doc comments (`OCCTBridge_Internal.h` on `occtPlateApproxSurface`, `OCCTBridge_Healing.mm` on `OCCTShapeFillBuildResult`) record why, so the same fix is not silently re-attempted after a future file breakdown (#393-#395) moves either comment. `ShapeUpgrade_UnifySameDomain`'s two bridge sites and `ShapeCustom_BSplineRestriction` were also swept and need nothing: the former exposes no error API at all (confirms #741's finding), the latter already self-polices in the kernel (declines a face's conversion outright rather than ever accepting one out of tolerance). See [`Scripts/repro/597-bridge-modeling-healing-approx-error/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-bridge-modeling-healing-approx-error) for the full measurement and [`Scripts/repro/572-approx-consumer-sweep/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/572-approx-consumer-sweep) for Cluster E's own census artifact. #597. - `BRepOffsetAPI_ThruSections::MakeSolid` marks a loft `Closed(true)` even when it could not actually cap both ends — **#905**. `ThruSectionsBuilder(isSolid: true)` silently omits both end-cap faces for a closed section wire with `k >= 2` periods of out-of-plane variation around the loop (a genuinely non-planar closed curve, not just one with nonzero Z spread): `build()` returns `true`, `shape.checkResult.isValid` is `false`, and `errorCount`/`detailedCheckStatuses` localize nothing. `MakeSolid()` caps each end via `PerformPlan()`, which only fits a plane (`BRepBuilderAPI_FindPlane`) or reuses a surface already attached to the wire's edges (`BRepLib_FindSurface`-backed `MakeFace(wire)`); neither fits a `k >= 2` wire (`k == 1`, e.g. `z = amp * cos(theta)` at constant radius, is secretly planar — the cylinder's intersection with a tilted plane — so it caps fine). `MakeSolid()` already tracks capping success in its own local `B`, threaded through both `PerformPlan()` calls, and discards it: it marks the shell/solid `Closed(true)` unconditionally regardless. Root cause behind #702 (closed): #702 fixed `healed()`/`fixSolid()` silently demoting the resulting invalid solid to a shell while reporting `isValid == true`; this issue is the capping omission #702 never investigated. **A null-face check (`myFirst.IsNull() || myLast.IsNull()`) looks like the fix and is not**: `PerformPlan()` also leaves the output face null, and returns success, when every edge of the wire is degenerate (a single-vertex "point" section from `AddVertex()`, e.g. a cone's apex) — that end needs no cap at all, and a null-face check cannot tell that apart from a genuine failure. Caught by CI on a fork PR staged to catch exactly this before submitting upstream: `BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge` (a circle-to-vertex loft, always legitimately capped) regressed to `IsDone() == false`; reproduced locally byte-for-byte (same failure line and message) before writing the real fix. **Fixed** (`Scripts/patches/0026-*`, override-link validated, not yet in a rebuilt xcframework): after the capping block, nullify both output faces (they alias the caller's `myFirst`/`myLast`, exposed via `FirstShape()`/`LastShape()`; a partial success on one wire before the other fails would otherwise leave a real, never-added face observable after a failed `Build()` — found in this PR's own review, unreachable via OCCTSwift's bridge but a real contract hazard for any other caller) and `throw StdFail_NotDone(...)`, matching the exception this same function already throws for a null shell two lines above — no signature change, no call-site change, since both call sites already run inside `Build()`'s only `try`/`catch`. `GetStatus()` is deliberately left unfixed: it stays `Done` on this path, since fixing it needs either a new out-parameter or a local `try`/`catch` at both call sites (the same multi-site shape the rejected null-face guard had) for a value nothing in this tree reads — `IsDone()` is the correct, and the only checked, signal. Validated against the actual upstream `BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge` (not just an equivalent local test): passes against the patched override, confirming the fix does not reintroduce the first attempt's regression. New GTests in `BRepOffsetAPI_ThruSections_Test.cxx`: `NonPlanarClosedWireCappingFails` (the `k=2` defect, proven to fail against the unpatched override first) and `DegenerateVertexEndStillSucceeds` (the cone/apex regression guard). Filed upstream as [OCCT#1462](https://github.com/Open-Cascade-SAS/OCCT/pull/1462), CI green on all platforms. **Process note**: the fork-PR staging step above ([gsdali/OCCT#1](https://github.com/gsdali/OCCT/pull/1), that fork's first and only PR) was itself a mistake — no other carried patch was ever validated that way, all went straight from local override-link testing to the real upstream PR — and is closed as erroneous; see `Scripts/patches/README.md`'s `0026` entry. **No general capping fix exists here or upstream**: a caller hitting this on a real non-planar loft (e.g. a bevel-gear section) can still assemble one by composition — loft the wall only (`isSolid: false`), pull the open boundary wires via `Shape.freeBoundsOpenWires`, cap each with `FillingSurface`/`Shape.fill` (#430/#433/#434), then sew — since `BRepFill_Filling`'s N-sided patch isn't limited to a plane the way `ThruSections`' own capping is. +- `BRepOffsetAPI_ThruSections::CreateSmoothed` overruns (or, in the opposite direction, silently + misaligns) its fixed-stride `shapes` array when a section's edge count differs from section 1's — + **#913**, found incidentally reviewing #910's own fix. `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. `checkCompatibility(true)` (the default) reconciles + differing edge counts via `BRepFill_CompatibleWires` before `CreateSmoothed()` ever runs; + `checkCompatibility(false)` skips that, so a later section with a different edge count than + section 1 either overruns the array (more edges — heap corruption, observed as SIGSEGV **and** + SIGBUS in different binaries, both genuinely reproduced, not a contradiction to resolve to one — + see the patch's own writeup) or, with fewer edges landing the running write index back in bounds + by the end, reports `Build() == true` with per-section strides silently misaligned to the wrong + geometry (confirmed: `Shape().IsValid() == false` for the accepted result). **Reached only with + 3+ sections**: exactly 2 always takes the `CreateRuled()` path instead, a different mechanism + that doesn't share this allocation. **Needs no reused builder**: the crash "needing" one was a + symptom of allocator state at the moment of the overrun, not the cause — a single `Build()` call + with all mismatched sections from the start carries the identical latent corruption, just + landing in unmapped-but-harmless heap slack more often than not. **Fixed** (`Scripts/patches/0027-*`, + override-link validated, not yet in a rebuilt xcframework): before allocating `shapes`, walk every + non-punctual section and reject on a count mismatch (an inequality test, not a "too many" test — + deliberately symmetric, since both directions are the same contract violation) with + `BRepFill_ThruSectionErrorStatus_ProfilesInconsistent`, matching the early-return idiom this + function already uses two lines below. Punctual end sections (`AddVertex()`, one degenerate edge, + not zero — an earlier draft of the comment said "zero" and was wrong) stay exempt, matching the + existing `w1Point`/`w2Point` handling; the exemption additionally verifies the section actually + has at least one edge before trusting that classification, since `w1Point`/`w2Point` are + vacuously `true` for a wire with no edges at all — a hardening this project could not reproduce a + live crash for (tried a fresh and a reused builder both; something downstream already reports + `IsDone() == false` for that input today, by an unconfirmed mechanism, independent of this patch) + but kept anyway since it is correct and cheap regardless. Filed upstream as + [OCCT#1466](https://github.com/Open-Cascade-SAS/OCCT/pull/1466), CI green. See + `Scripts/patches/README.md`'s `0027` entry for the full writeup, including the two rejected + test-fixture arithmetic mistakes found en route (a degenerate 2-edge "digon" test that + accidentally exercised an unrelated shortfall case instead of the reviewer's own carefully-chosen + no-overrun example, and a zero-edge GTest that was removed rather than kept as unproven coverage). ### Carrying OCCT source patches diff --git a/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch b/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch index 0ffa66c26..19fba527b 100644 --- a/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch +++ b/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch @@ -1,6 +1,6 @@ From: OCCTSwift ecosystem Subject: [PATCH] Modeling Algorithms - guard CreateSmoothed's fixed-stride - shapes array against a section with more edges than section 1 + shapes array against a mismatched section edge count BRepOffsetAPI_ThruSections::CreateSmoothed() derives `nbEdges` -- the edge count it assumes every section has -- from section 1 alone (or section 2, if @@ -13,49 +13,84 @@ Nothing enforces that every section actually has `nbEdges` edges. BRepFill_CompatibleWires (via CheckCompatibility(true), the default) normally reconciles differing edge counts across sections before CreateSmoothed() ever runs. With CheckCompatibility(false) ("no check"), -that reconciliation 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 a later SIGSEGV inside -CreateSmoothed() itself once at least 3 sections are involved (2 sections -always take the CreateRuled() path instead, which does not share this -allocation shape). Confirmed via a minimal, from-scratch C++ reproducer with -no OCCTSwift/bridge involvement: two matching circle sections build -successfully, reusing the same builder with a third, differently-shaped -section (more edges than the first two) and rebuilding SIGSEGVs on the -stock library. Needs no reused builder as such -- what actually varies is -process/allocator state at the time of the overrun, which is why an -otherwise-identical single Build() call with all three sections added up +that reconciliation is skipped entirely, so a later section with a +DIFFERENT edge count than section 1 either: + + - has MORE edges: the fill loop walks past the end of `shapes` -- an + out-of-bounds write, observed as heap corruption and a later SIGSEGV + inside CreateSmoothed() itself once at least 3 sections are involved (2 + sections always take the CreateRuled() path instead, which does not + share this allocation shape); or + - has FEWER edges: no overrun (the total write count can still land inside + `shapes`' bounds), so Build() reports success today -- but with + per-section strides silently misaligned to different geometry than + intended, an incorrect result with no signal anything went wrong. + +Confirmed via a minimal, from-scratch C++ reproducer with no OCCTSwift/ +bridge involvement: two matching circle sections build successfully, +reusing the same builder with a third, differently-shaped section (more +edges than the first two) and rebuilding SIGSEGVs on the stock library. +Needs no reused builder as such for the overrun case -- what actually +varies is process/allocator state at the time of the overrun, which is why +an otherwise-identical single Build() call with all three sections added up front does not reliably crash even though the same out-of-bounds write -still occurs. +still occurs. The fewer-edges case does not depend on process state at all: +confirmed directly that a 2/1/3-edge three-section input under +CheckCompatibility(false) reports Build() == true and Shape().IsValid() == +false against the stock library, every time. Fix: before allocating `shapes`, walk every non-punctual section and count its edges; if any section's count differs from `nbEdges`, set myStatus to BRepFill_ThruSectionErrorStatus_ProfilesInconsistent and return, matching the early-return idiom this function already uses two lines below -(TS.IsNull() -> myStatus = Failed; return;). Punctual end sections (a -degenerate vertex added via AddVertex(), e.g. a cone's apex) are exempt, -matching the existing w1Point/w2Point handling throughout the rest of the -function -- a point section legitimately has a different edge count and is -not itself walked by the fill loop's "punctual" branch. +(TS.IsNull() -> myStatus = Failed; return;). This is symmetric by +construction (an inequality test, not a "too many" test): it also rejects +the previously-silent fewer-edges case, which is intentional -- both +directions are the same underlying contract violation, only one of them +used to crash. + +Punctual end sections (a degenerate vertex added via AddVertex(), e.g. a +cone's apex) are exempt, matching the existing w1Point/w2Point handling +throughout the rest of the function -- a point section legitimately has a +different edge count and is not itself walked by the fill loop's "punctual" +branch the same way. That branch repeats the section's one degenerate edge +nbEdges times to fill its slots; it does not merely skip validation, so a +punctual section still needs at least that one edge to exist. w1Point/ +w2Point are computed by an existing loop that is vacuously true for a wire +with NO edges at all (the loop body that would clear it never runs), which +would let the fill loop below walk an uninitialized BRepTools_WireExplorer +and read a null edge -- not reachable through any wrapper this project +ships (a wire needs at least one edge to exist there), but the guard added +here checks for at least one edge before exempting a section as punctual, +rather than trusting w1Point/w2Point's classification unconditionally. + +The guard shares its punctual-section test (`isPunctualSection`, a local +lambda) with the pre-existing fill loop 15 lines below, which used to +duplicate the same two-clause boolean expression separately -- hoisted so +the two cannot silently drift apart on which sections take the punctual +branch. Validated by override-linking the patched translation unit ahead of the stock archive (both with and without No_Exception/NDEBUG, matching the -production Release configuration this project ships): the original 3 -scenarios that legitimately succeed today (checkCompatibility(false) with +production Release configuration this project ships) across six scenarios: +the three that legitimately succeed today (checkCompatibility(false) with genuinely matching section edge counts; a punctual section at either end -mixed with matching wire sections; checkCompatibility(true) with -mismatched sections, reconciled by BRepFill_CompatibleWires as before) are -all byte-for-byte unaffected, and the crashing scenario now fails cleanly -(IsDone() == false) instead of crashing. +mixed with matching wire sections; checkCompatibility(true) with mismatched +sections, reconciled by BRepFill_CompatibleWires as before) are all +byte-for-byte unaffected; the original crashing scenario (more edges) now +fails cleanly (IsDone() == false) instead of crashing; a new fewer-edges +scenario, silently "successful" with invalid geometry on the stock library, +now also fails cleanly; and a section with genuinely zero edges at the +punctual position no longer reaches the fill loop's unguarded walk. SecondMouseAU/OCCTSwift#913. --- - .../TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx | 23 +++++++++++++++++++++ - 1 file changed, 23 insertions(+) + .../TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx | 46 +++++++++++++++++++++++++++++++++++++++++++++- + 1 file changed, 45 insertions(+), 1 deletion(-) --- a/src/ModelingAlgorithms/TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx +++ b/src/ModelingAlgorithms/TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.cxx -@@ -747,6 +747,29 @@ +@@ -747,6 +760,51 @@ void BRepOffsetAPI_ThruSections::CreateSmoothed() } } @@ -63,12 +98,34 @@ SecondMouseAU/OCCTSwift#913. + // skips BRepFill_CompatibleWires, so nothing else guarantees every other section has the same + // edge count -- and the fixed-stride fill loop below indexes `shapes` on that assumption with no + // bounds check, overrunning it (heap corruption, observed as a later SIGSEGV) for a section with -+ // more edges than section 1. Refuse cleanly instead of writing out of bounds. ++ // more edges than section 1, or silently misaligning per-section strides (no crash, but a wrong ++ // result) for one with fewer. Refuse cleanly instead. Shared with the fill loop's own identical ++ // test below, so the two can't drift apart on which sections take the punctual branch. ++ auto isPunctualSection = [&](int theIndex) { ++ return (theIndex == 1 && w1Point) || (theIndex == nbSects && w2Point); ++ }; + for (int iSect = 1; iSect <= nbSects; iSect++) + { -+ if ((iSect == 1 && w1Point) || (iSect == nbSects && w2Point)) ++ if (isPunctualSection(iSect)) + { -+ continue; // a punctual end section legitimately has a different (zero) edge count ++ // A punctual end section (AddVertex()) has exactly ONE degenerate edge, which the fill ++ // loop below repeats nbEdges times to fill that section's slots -- not zero edges. w1Point/ ++ // w2Point are also (vacuously) true for a wire with NO edges at all, which the fill loop ++ // would then walk with an uninitialized BRepTools_WireExplorer, reading a null edge. Not ++ // reachable through any wrapper this project ships (a wire needs at least one edge to ++ // exist), but a punctual classification should not admit that case either. ++ bool hasAnyEdge = false; ++ for (anExp.Init(TopoDS::Wire(myWires(iSect))); anExp.More(); anExp.Next()) ++ { ++ hasAnyEdge = true; ++ break; ++ } ++ if (!hasAnyEdge) ++ { ++ myStatus = BRepFill_ThruSectionErrorStatus_ProfilesInconsistent; ++ return; ++ } ++ continue; + } + int aSectEdges = 0; + for (anExp.Init(TopoDS::Wire(myWires(iSect))); anExp.More(); anExp.Next()) @@ -85,3 +142,12 @@ SecondMouseAU/OCCTSwift#913. // recover the shapes bool uClosed = true; NCollection_Array1 shapes(1, nbSects * nbEdges); +@@ -765,7 +823,7 @@ void BRepOffsetAPI_ThruSections::CreateSmoothed() + uClosed = false; + } + } +- if ((i == 1 && w1Point) || (i == nbSects && w2Point)) ++ if (isPunctualSection(i)) + { + // if the wire is punctual + anExp.Init(TopoDS::Wire(wire)); diff --git a/Scripts/patches/README.md b/Scripts/patches/README.md index 2fd709c72..0af10e363 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -779,44 +779,96 @@ each section's wire with a `BRepTools_WireExplorer`, incrementing a running inde check**. Nothing enforces that every section actually has `nbEdges` edges: `BRepFill_CompatibleWires` (via `checkCompatibility(true)`, the default) normally reconciles differing edge counts across sections before `CreateSmoothed()` ever runs, but `checkCompatibility(false)` skips that entirely. -A later section with **more** edges than section 1 walks the fill loop straight past the end of -`shapes` — an out-of-bounds write, corrupting adjacent heap memory rather than raising a catchable -failure. +A section with a **different** edge count than section 1 goes one of two ways: + +- **More edges**: the fill loop walks the array straight past the end of `shapes` — an + out-of-bounds write, corrupting adjacent heap memory rather than raising a catchable failure. +- **Fewer edges**: no overrun (the total write count can still land inside `shapes`' bounds), so + `Build()` reports success today — but with per-section strides silently misaligned to different + geometry than intended. Confirmed directly: a 2/1/3-edge three-section input under + `checkCompatibility(false)` reports `Build() == true` and `Shape().IsValid() == false` against + the stock library, deterministically, no process-state dependency (PR #915 review finding 3 — + the first version of this patch's own SemVer note undersold this, describing only the crashing + direction). **Reached only with 3+ sections.** With exactly 2 sections, `Build()` always takes the `CreateRuled()` path instead (`if (myWires.Length() == 2 || myIsRuled) CreateRuled(); else CreateSmoothed();`), which builds its shell via `BRepFill_Generator` — a different mechanism that -doesn't share this fixed-stride allocation. A single `Build()` call with all mismatched sections -present from the start does **not** reliably crash even though the identical out-of-bounds write -still occurs; what actually varies is process/allocator state at the time of the overrun (confirmed -directly: instrumenting the fill loop shows the write index reaching one past `shapes.Upper()` -either way, but a from-scratch process quietly lands the write in unmapped-but-harmless heap slack -where a process that already ran one successful `Build()` call does not). Symptom, not cause: this -made the defect look like it needed a *reused* builder when it doesn't — any 3+-section -`checkCompatibility(false)` call with mismatched edge counts carries the same latent corruption. +doesn't share this fixed-stride allocation. A single `Build()` call with all mismatched (more-edges) +sections present from the start does **not** reliably crash even though the identical out-of-bounds +write still occurs; what actually varies is process/allocator state at the time of the overrun +(confirmed directly: instrumenting the fill loop shows the write index reaching one past +`shapes.Upper()` either way, but a from-scratch process quietly lands the write in +unmapped-but-harmless heap slack where a process that already ran one successful `Build()` call does +not). Symptom, not cause: this made the defect look like it needed a *reused* builder when it +doesn't — any 3+-section `checkCompatibility(false)` call with mismatched edge counts carries the +same latent corruption (or, in the fewer-edges direction, the same silent misalignment, +deterministically regardless of process state). **Fix:** before allocating `shapes`, walk every non-punctual section and count its edges; on a -mismatch, set `myStatus` to `BRepFill_ThruSectionErrorStatus_ProfilesInconsistent` and return, -matching the early-return idiom this function already uses two lines below (`TS.IsNull()` -> -`myStatus = Failed; return;`). Punctual end sections (`AddVertex()`, e.g. a cone's apex) are exempt, -matching the existing `w1Point`/`w2Point` handling throughout the rest of the function — a point -section legitimately has a different (zero) edge count and the fill loop's punctual branch doesn't -walk it the same way. +mismatch (an inequality test, not a "too many" test — deliberately symmetric, since both directions +are the same underlying contract violation and only one of them used to crash), set `myStatus` to +`BRepFill_ThruSectionErrorStatus_ProfilesInconsistent` and return, matching the early-return idiom +this function already uses two lines below (`TS.IsNull()` -> `myStatus = Failed; return;`). Punctual +end sections (`AddVertex()`, e.g. a cone's apex) are exempt, matching the existing +`w1Point`/`w2Point` handling throughout the rest of the function — a point section legitimately has +a different edge count and the fill loop's punctual branch doesn't walk it the same way: that branch +repeats the section's own edge `nbEdges` times to fill its slots, so it needs at least one edge to +exist, not zero (PR #915 review finding 11 — an earlier draft of this entry, and the patch's own +first-draft comment, said "(zero) edge count"; `AddVertex()` creates a wire with exactly **one** +degenerate edge). `w1Point`/`w2Point` themselves are computed by a pre-existing loop that is +vacuously `true` for a wire with no edges at all — not reachable through any wrapper this project +ships (a `Wire` needs at least one edge to exist), but the guard checks for at least one edge before +exempting a section as punctual rather than trusting that classification unconditionally (PR #915 +review finding 4; attempted to reproduce a live crash for this specific case via both a fresh and a +reused builder and could not — OCCT's own pipeline handles a genuinely empty wire more gracefully +than the code reading alone suggested — but the added check is correct and cheap regardless of +whether today's fill loop actually reaches the unguarded path it describes). + +The guard shares its punctual-section test (`isPunctualSection`, a local lambda) with the +pre-existing fill loop 15 lines below, which used to duplicate the same two-clause boolean +expression separately (PR #915 review finding 9) — hoisted so the two cannot silently drift apart on +which sections take the punctual branch. **Validation** (override-link, no full rebuild for this patch's own writeup — see `#0001`'s retired entry for the technique; the OCCTSwift-side PR carrying this one also rebuilds the local xcframework, since #913 asked for that explicitly rather than deferring it like `0026`): compiled -and linked both the unpatched and patched `.cxx` ahead of the pinned `libOCCT-macos.a`. Against the -unpatched override, a new GTest (`BRepOffsetAPI_ThruSections_Test.cxx`, -`MismatchedSectionEdgeCountFailsCleanlyWithoutCheck`) crashes (SIGBUS) exactly as the issue -describes; against the patched override it passes (`IsDone() == false`, no crash), along with a -second new regression-guard test (`MatchingSectionEdgeCountsStillSucceedWithoutCheck`, matching -edge counts including a punctual end section) and all three pre-existing tests in the file — 5/5. -Re-confirmed the fix holds with `No_Exception`/`NDEBUG` defined (matching a Release build -configuration close to what the shipped archive itself was likely built with): the fix eliminates -the out-of-bounds write itself, so it does not depend on `Standard_OutOfRange`'s range check being -compiled in. `clang-format --dry-run --Werror` against OCCT's own `.clang-format` reports zero -violations on both changed files. +and linked both the unpatched and patched `.cxx` ahead of the pinned `libOCCT-macos.a`, across six +scenarios — the three that legitimately succeed today (matching edge counts under +`checkCompatibility(false)`; a punctual section at either end mixed with matching wire sections; +`checkCompatibility(true)` with mismatched sections, reconciled by `BRepFill_CompatibleWires` as +before) are all byte-for-byte unaffected; the original more-edges scenario now fails cleanly +(`IsDone() == false`) instead of crashing; the new fewer-edges scenario, silently "successful" with +invalid geometry on the stock library, now also fails cleanly; and a section with genuinely zero +edges at the punctual position no longer reaches the fill loop's unguarded walk. Re-confirmed all +six hold with `No_Exception`/`NDEBUG` defined (matching a Release build configuration close to what +the shipped archive itself was likely built with): the fix eliminates the out-of-bounds write +itself, so it does not depend on `Standard_OutOfRange`'s range check being compiled in. +`clang-format --dry-run --Werror` against OCCT's own `.clang-format` reports zero violations on +both changed files. + +**The zero-edge-at-punctual-position scenario is a code hardening, not a proven-live fix.** +Committed only the fewer-edges GTest, not a zero-edge one: the fewer-edges scenario genuinely fails +`EXPECT_FALSE(IsDone())` against the pristine, unpatched source (proved first, per this project's +own "prove the test fails" convention, before writing the fix), but a from-scratch equivalent for a +genuinely empty wire at the punctual position passes *even against pristine, unpatched code* — +something else downstream (most likely `TotalSurf()`'s own null-surface guard, a few lines below, +reacting to whatever a degenerate empty-wire input produces) already reports `IsDone() == false` +for it today, by a different and unconfirmed mechanism, independent of this patch. The explicit +`hasAnyEdge` check is kept anyway (it is correct regardless, and cheap), but no committed test +claims to be proof it closes a live gap, because the one written could not be made to fail without +it — see finding 4 in the PR #915 review thread for the full attempt, including a reused-builder +variant that also did not reproduce a crash. + +**SIGSEGV vs. SIGBUS** (PR #915 review finding 12): both signals were genuinely observed for the +more-edges overrun, in different binaries, and that is not a contradiction to reconcile down to one +— it's exactly what heap corruption looks like. The standalone from-scratch C++ reproducer (its own +`backtrace_symbols_fd`-based signal handler installed) reported signal 11 (SIGSEGV) consistently. +The upstream GTest (`MismatchedSectionEdgeCountFailsCleanlyWithoutCheck`, linked against the stock +archive with no custom handler, OS default handling) reported signal 10 (SIGBUS). Same defect, same +out-of-bounds write, two different binaries with different allocator/memory layout at the moment of +the overrun — which of the two manifests is exactly the kind of detail this defect's own root-cause +section says depends on process/allocator state, not something either transcript got wrong. Filed upstream as [OCCT#1466](https://github.com/Open-Cascade-SAS/OCCT/pull/1466). diff --git a/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/README.md b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/README.md new file mode 100644 index 000000000..9c9249800 --- /dev/null +++ b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/README.md @@ -0,0 +1,119 @@ +# OCCTSwift #913: `BRepOffsetAPI_ThruSections::CreateSmoothed()` has no bounds check on its fixed-stride `shapes` array + +`ThruSectionsBuilder`, given `checkCompatibility(false)` and 3+ sections where a later section's +edge count differs from section 1's, either SIGSEGVs/SIGBUSes (more edges) or silently succeeds +with wrong geometry (fewer edges) instead of failing cleanly. Found incidentally while hunting for +a failure trigger during [#910](https://github.com/SecondMouseAU/OCCTSwift/issues/910)'s own PR +review. + +## Mechanism + +`CreateSmoothed()` derives `nbEdges` -- the edge count it assumes every section has -- from +section 1 alone (or section 2, if section 1 is a punctual/degenerate vertex section). It allocates +`shapes`, an `NCollection_Array1` sized exactly `nbSects * nbEdges`, then fills it by +walking each section's wire with a `BRepTools_WireExplorer`, incrementing a running index with +**no bounds check**. + +`BRepFill_CompatibleWires` (via `checkCompatibility(true)`, the default) normally reconciles +differing edge counts across sections before `CreateSmoothed()` ever runs. `checkCompatibility +(false)` ("no check") skips that entirely, so nothing enforces that every section actually has +`nbEdges` edges. Two ways that goes wrong, both exercised in `occt_913_test.mm` below: + +- **More edges than section 1**: the fill loop walks the array straight past the end of `shapes` + -- an out-of-bounds write, corrupting adjacent heap memory instead of raising a catchable + failure. +- **Fewer edges than section 1**: if a later section's surplus edges exactly compensate an + earlier section's shortfall, the running write index never exceeds the array's bounds at any + point -- no overrun, no crash, so `Build()` reports success today, but with per-section strides + silently misaligned to the wrong geometry. Confirmed: `Build() == true` **and** + `Shape().IsValid() == false` for the accepted result. + +**Reached only with 3+ sections.** With exactly 2 sections, `Build()` always takes the +`CreateRuled()` path instead (`if (myWires.Length() == 2 || myIsRuled) CreateRuled(); else +CreateSmoothed();`), which builds its shell via `BRepFill_Generator` -- a different mechanism that +doesn't share this fixed-stride allocation. + +**Needs no reused builder as such.** The original investigation found the crash only via a +"build once successfully, reuse the same builder, build again with a mismatched section" sequence, +which looked like it needed that specific reuse pattern. It doesn't: instrumenting the fill loop +directly shows the write index reaching one past `shapes.Upper()` on a single, from-scratch +`Build()` call too. What actually varies is process/allocator state at the moment of the overrun -- +a from-scratch process more often lands the out-of-bounds write in unmapped-but-harmless heap slack +than a process that has already done other allocation/deallocation work, so a single-call +reproduction is unreliable while a reused-builder one (or, as this probe's own runs show, one +preceded by several other successful `ThruSections` builds) is *more* likely to crash but still not +guaranteed to, every run. + +**SIGSEGV and SIGBUS were both genuinely observed, in different binaries -- not a contradiction to +resolve to one signal.** An early, isolated reproducer (just the reused-builder overrun scenario, +nothing else run first) crashed with `signal 11` (SIGSEGV) consistently, caught by this probe's own +`backtrace_symbols_fd`-based handler. A separately-built GTest binary (linking +`BRepOffsetAPI_ThruSections_Test.cxx` against the same stock archive, no custom handler, OS default +signal handling) crashed with `Bus error: 10` (SIGBUS). CI caught a third instance directly: PR +#915's `swift build + test (macOS)` job (`swift test` against `Package.swift`'s pinned kernel, +which does not have this patch) aborted with: + +``` +*** Abort *** an exception was raised, but no catch was found. + ... The exception is: SIGSEGV 'segmentation violation' detected. Address 85e5c2a64238. +``` + +Same defect, same out-of-bounds write, three different binaries -- which signal manifests depends +on allocator/memory layout at the moment of the overrun, exactly the process-state sensitivity +described above. Do not read a different signal on a later observation as evidence this is a +different bug. + +## Running it + +```bash +clang++ -std=c++17 -ObjC++ -w \ + -I"Libraries/OCCT.xcframework/macos-arm64/Headers" \ + -L"Libraries/OCCT.xcframework/macos-arm64" \ + -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ \ + Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/occt_913_test.mm \ + -o /tmp/occt_913_test +/tmp/occt_913_test +``` + +Compile once against the stock archive (`stock.txt`) and once with a patched +`BRepOffsetAPI_ThruSections.cxx` override-linked in front -- compile that file standalone and link +its `.o` **before** `-lOCCT-macos` on the command line (`patched.txt`) -- and diff. Six scenarios, +matching the six numbered cases in the probe's own header comment: three that must always succeed +(matching edge counts, a punctual apex, and `checkCompatibility(true)` reconciliation), the overrun +case (case 4 -- process-state dependent, see above; this exact captured run did not crash, which +is itself expected and documented, not a failure of the repro), the fewer-edges silent-success case +(case 5 -- the one difference between `stock.txt` and `patched.txt`: `IsDone=1` before, `IsDone=0` +after), and the zero-edge punctual hardening (case 6 -- `IsDone=0` on both, see below). + +## The zero-edge-at-punctual-position case is a hardening, not a proven-live fix + +The patch's punctual-section exemption (`w1Point`/`w2Point`) additionally checks that the section +actually has at least one edge before exempting it from validation, since `w1Point`/`w2Point` are +computed by a pre-existing loop that is vacuously `true` for a wire with **no** edges at all (the +loop body that would clear the flag never runs) -- which would otherwise let the fill loop walk an +uninitialized `BRepTools_WireExplorer` and read a null edge. + +Attempted to reproduce a live crash for this specific case, including via a reused builder (build +successfully with 3 matching sections, then add a genuinely empty wire as a 4th and rebuild) -- see +case 6 in `occt_913_test.mm`. Neither attempt crashed, against stock OR patched: something else +downstream (most likely `TotalSurf()`'s own null-surface guard reacting to whatever a degenerate +empty-wire input produces) already reports `IsDone() == false` for this input today, by an +unconfirmed mechanism independent of this patch. The hardening is kept anyway -- it is correct and +cheap regardless of whether today's fill loop actually reaches the unguarded path it describes -- +but no committed GTest claims to prove it closes a live gap, because the one written for it could +not be made to fail without the check and was removed rather than kept as unproven coverage. + +## Fix + +`Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch`. +Before allocating `shapes`, walk every non-punctual section and count its edges; on a mismatch (an +inequality test, not a "too many" test -- deliberately symmetric, since both directions are the +same underlying contract violation), set `myStatus` to +`BRepFill_ThruSectionErrorStatus_ProfilesInconsistent` and return, matching the early-return idiom +this function already uses two lines below. The punctual-section test (`isPunctualSection`, a local +lambda) is shared with the pre-existing fill loop 15 lines below, which used to duplicate the same +two-clause boolean expression separately. + +Filed upstream as [OCCT#1466](https://github.com/Open-Cascade-SAS/OCCT/pull/1466). Full validation +transcripts and the PR #915 review response (12 findings, all addressed or explicitly documented) +live in `Scripts/patches/README.md`'s `0027` entry. diff --git a/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/occt_913_test.mm b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/occt_913_test.mm new file mode 100644 index 000000000..d2cc7c478 --- /dev/null +++ b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/occt_913_test.mm @@ -0,0 +1,186 @@ +// Ground truth probe for OCCTSwift#913: BRepOffsetAPI_ThruSections::CreateSmoothed() has no +// bounds check on its fixed-stride `shapes` array, which is sized from section 1's edge count +// alone. Under checkCompatibility(false), a section with a DIFFERENT edge count than section 1 +// either overruns the array (more edges -- heap corruption, SIGSEGV/SIGBUS) or, if the running +// write index still lands in bounds by the end, silently misaligns per-section strides (fewer +// edges -- Build() == true, Shape().IsValid() == false, no signal anything went wrong). +// +// This probe runs six cases and prints IsDone()/crash status for each: +// +// 1. matching: 3 matching-edge-count sections, checkCompatibility(false) -- must succeed. +// 2. punctual: an apex (AddVertex) + 2 matching wire sections -- must succeed (the +// w1Point/w2Point exemption must not over-reject a legitimate cone-apex loft). +// 3. reconciled: mismatched sections WITH checkCompatibility(true) (default) -- must succeed, +// unaffected (BRepFill_CompatibleWires reconciles before CreateSmoothed runs). +// 4. overrun: a reused builder, 2 matching circles then a 3rd section with MORE edges, +// checkCompatibility(false) -- the original crash. Stock: SIGSEGV (this probe's +// own signal handler) or SIGBUS (see README -- both genuinely observed, in +// different binaries). Patched: IsDone() == false, no crash. +// 5. fewer: 3 sections (2, 1, 3 edges -- the reviewer's own carefully-chosen example, +// where the running write index never exceeds bounds because the shortfall in +// section 2 is exactly made up by the surplus in section 3), checkCompatibility +// (false). Stock: IsDone() == true, Shape().IsValid() == false (silent wrong +// answer). Patched: IsDone() == false. +// 6. emptyPunctual: a genuinely zero-edge wire at the punctual position. Stock and patched both +// report IsDone() == false and neither crashes -- this project could not +// reproduce a live crash for this case (see README); the patch's explicit +// hasAnyEdge check is a hardening kept for correctness, not proof of closing an +// observed defect. +// +// Compile once against the stock archive and once with a patched BRepOffsetAPI_ThruSections.cxx +// override-linked in front (see README.md), and diff. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace +{ +void crashHandler(int sig) +{ + void* frames[64]; + int n = backtrace(frames, 64); + fprintf(stderr, "\n=== crashHandler: signal %d ===\n", sig); + backtrace_symbols_fd(frames, n, STDERR_FILENO); + _exit(134); +} + +TopoDS_Wire circleWire(double theRadius, double theZ) +{ + gp_Ax2 anAxis(gp_Pnt(0, 0, theZ), gp::DZ()); + gce_MakeCirc aMakeCirc(anAxis, theRadius); + BRepBuilderAPI_MakeEdge aMakeEdge(aMakeCirc.Value()); + return BRepBuilderAPI_MakeWire(aMakeEdge.Edge()).Wire(); +} + +TopoDS_Wire nGonWire(int theN, double theRadius, double theZ) +{ + BRepBuilderAPI_MakePolygon aPolygon; + for (int i = 0; i < theN; ++i) + { + double anAngle = 2.0 * M_PI * i / theN; + aPolygon.Add(gp_Pnt(theRadius * cos(anAngle), theRadius * sin(anAngle), theZ)); + } + aPolygon.Close(); + return aPolygon.Wire(); +} + +TopoDS_Vertex apexVertex(double theZ) +{ + return BRepBuilderAPI_MakeVertex(gp_Pnt(0, 0, theZ)); +} + +TopoDS_Wire emptyWire() +{ + TopoDS_Wire w; + BRep_Builder b; + b.MakeWire(w); + return w; +} +} // namespace + +int main() +{ + signal(SIGSEGV, crashHandler); + signal(SIGBUS, crashHandler); + signal(SIGABRT, crashHandler); + + // 1. matching + { + BRepOffsetAPI_ThruSections ts(true, false); + ts.CheckCompatibility(false); + ts.AddWire(nGonWire(4, 5.0, 0.0)); + ts.AddWire(nGonWire(4, 4.0, 5.0)); + ts.AddWire(nGonWire(4, 3.0, 10.0)); + ts.Build(); + printf("1. matching (3x 4-gon, no-check): IsDone=%d (want 1)\n", ts.IsDone()); + fflush(stdout); + } + + // 2. punctual + { + BRepOffsetAPI_ThruSections ts(true, false); + ts.CheckCompatibility(false); + ts.AddVertex(apexVertex(0.0)); + ts.AddWire(nGonWire(5, 4.0, 5.0)); + ts.AddWire(nGonWire(5, 3.0, 10.0)); + ts.Build(); + printf("2. punctual (apex + 2x 5-gon, no-check): IsDone=%d (want 1)\n", ts.IsDone()); + fflush(stdout); + } + + // 3. reconciled + { + BRepOffsetAPI_ThruSections ts(true, false); + ts.CheckCompatibility(true); + ts.AddWire(circleWire(5.0, 0.0)); + ts.AddWire(circleWire(3.0, 5.0)); + ts.AddWire(nGonWire(3, 2.0, 10.0)); + ts.Build(); + printf("3. reconciled (circle,circle,triangle, WITH check): IsDone=%d (want 1)\n", ts.IsDone()); + fflush(stdout); + } + + // 4. overrun (the original crash) + { + BRepOffsetAPI_ThruSections ts(true, false); + ts.CheckCompatibility(false); + ts.AddWire(circleWire(5.0, 0.0)); + ts.AddWire(circleWire(3.0, 10.0)); + ts.Build(); + printf("4. overrun: build #1 (2 circles): IsDone=%d\n", ts.IsDone()); + fflush(stdout); + ts.AddWire(nGonWire(3, 2.0, 20.0)); + printf("4. overrun: about to build #2 (+triangle, more edges, no-check)...\n"); + fflush(stdout); + ts.Build(); + printf("4. overrun: build #2: IsDone=%d (want 0, no crash)\n", ts.IsDone()); + fflush(stdout); + } + + // 5. fewer (the reviewer's own no-overrun example: 2 + 1 + 3 = 3 * 2) + { + BRepOffsetAPI_ThruSections ts(true, false); + ts.CheckCompatibility(false); + ts.AddWire(nGonWire(2, 5.0, 0.0)); + ts.AddWire(circleWire(3.0, 10.0)); + ts.AddWire(nGonWire(3, 2.0, 20.0)); + ts.Build(); + printf("5. fewer (2,1,3 edges, no-check): IsDone=%d (want 0)\n", ts.IsDone()); + fflush(stdout); + } + + // 6. emptyPunctual + { + BRepOffsetAPI_ThruSections ts(true, false); + ts.CheckCompatibility(false); + ts.AddWire(emptyWire()); + ts.AddWire(nGonWire(4, 4.0, 5.0)); + ts.AddWire(nGonWire(4, 3.0, 10.0)); + ts.Build(); + printf("6. emptyPunctual (empty wire + 2x matching 4-gon, no-check): IsDone=%d (want 0, no " + "crash on stock OR patched)\n", + ts.IsDone()); + fflush(stdout); + } + + printf("ALL DONE, no crash\n"); + return 0; +} diff --git a/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/patched.txt b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/patched.txt new file mode 100644 index 000000000..d89b4367b --- /dev/null +++ b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/patched.txt @@ -0,0 +1,9 @@ +1. matching (3x 4-gon, no-check): IsDone=1 (want 1) +2. punctual (apex + 2x 5-gon, no-check): IsDone=1 (want 1) +3. reconciled (circle,circle,triangle, WITH check): IsDone=1 (want 1) +4. overrun: build #1 (2 circles): IsDone=1 +4. overrun: about to build #2 (+triangle, more edges, no-check)... +4. overrun: build #2: IsDone=0 (want 0, no crash) +5. fewer (2,1,3 edges, no-check): IsDone=0 (want 0) +6. emptyPunctual (empty wire + 2x matching 4-gon, no-check): IsDone=0 (want 0, no crash on stock OR patched) +ALL DONE, no crash diff --git a/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/stock.txt b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/stock.txt new file mode 100644 index 000000000..3bd27c10e --- /dev/null +++ b/Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/stock.txt @@ -0,0 +1,9 @@ +1. matching (3x 4-gon, no-check): IsDone=1 (want 1) +2. punctual (apex + 2x 5-gon, no-check): IsDone=1 (want 1) +3. reconciled (circle,circle,triangle, WITH check): IsDone=1 (want 1) +4. overrun: build #1 (2 circles): IsDone=1 +4. overrun: about to build #2 (+triangle, more edges, no-check)... +4. overrun: build #2: IsDone=0 (want 0, no crash) +5. fewer (2,1,3 edges, no-check): IsDone=1 (want 0) +6. emptyPunctual (empty wire + 2x matching 4-gon, no-check): IsDone=0 (want 0, no crash on stock OR patched) +ALL DONE, no crash diff --git a/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift b/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift index 6c6045592..06fc3824b 100644 --- a/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift +++ b/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift @@ -436,10 +436,21 @@ struct StressThruSectionsBuilderLifecycleTests { // (reached only at 3+ sections — 2 sections always take the CreateRuled() path instead) walked // a fixed-stride array sized from section 1 alone with no bounds check, overrunning it and // SIGSEGVing for a later section with more edges than the first. Must fail cleanly instead. - @Test func mismatchedSectionEdgeCountWithoutCheckFailsCleanly() { - guard let w1 = Wire.circle(origin: SIMD3(0, 0, 0), normal: SIMD3(0, 0, 1), radius: 5), - let w2 = Wire.circle(origin: SIMD3(0, 0, 10), normal: SIMD3(0, 0, 1), radius: 3), - let s1 = Shape.fromWire(w1), let s2 = Shape.fromWire(w2) else { return } + // + // Gated on OCCTSWIFT_LOCAL (PR #915 review, finding 1): the fix ships as Scripts/patches/0027, + // not yet in Package.swift's pinned kernel asset. ci.yml's default `swift test` resolves that + // pinned kernel, where this exact scenario still SIGSEGVs for real — SwiftPM runs every test + // target in one process, so an unguarded run here would abort the whole suite, not just this + // test, indistinguishable from a real regression (the #585 failure shape). kernel-integration.yml + // sets OCCTSWIFT_LOCAL=1 when it builds Scripts/patches/ from source and runs against that + // binary instead — matching this repo's own convention, see #905/PR #909, which added no Swift + // test at all for the identical reason. This test only runs there, not against the pinned kernel. + @Test(.enabled(if: ProcessInfo.processInfo.environment["OCCTSWIFT_LOCAL"] == "1")) + func mismatchedSectionEdgeCountWithoutCheckFailsCleanly() throws { + let w1 = try #require(Wire.circle(origin: SIMD3(0, 0, 0), normal: SIMD3(0, 0, 1), radius: 5)) + let w2 = try #require(Wire.circle(origin: SIMD3(0, 0, 10), normal: SIMD3(0, 0, 1), radius: 3)) + let s1 = try #require(Shape.fromWire(w1)) + let s2 = try #require(Shape.fromWire(w2)) let loft = ThruSectionsBuilder(isSolid: true, isRuled: false) loft.checkCompatibility(false) loft.addWire(s1) @@ -447,12 +458,35 @@ struct StressThruSectionsBuilderLifecycleTests { #expect(loft.build()) // A third section with MORE edges (a triangle, 3) than the first two (1 each, circles). - guard let triangle = Wire.polygon3D([ + let triangle = try #require(Wire.polygon3D([ SIMD3(2, 0, 20), SIMD3(-1, 1.7320508, 20), SIMD3(-1, -1.7320508, 20) - ], closed: true), let triangleShape = Shape.fromWire(triangle) else { return } + ], closed: true)) + let triangleShape = try #require(Shape.fromWire(triangle)) loft.addWire(triangleShape) #expect(!loft.build()) } + + // #913 patch review (PR #915), finding 5: the w1Point/w2Point punctual-section exemption is + // the only thing keeping a cone-apex loft (addVertex(), public API) working once #913's guard + // reaches CreateSmoothed (3+ sections). The only existing addVertex() loft test + // (ThruSectionsGuardTests.singleVertexBuildReturnsFalse) is a single-vertex build that fails + // by design; nothing pinned a legitimate punctual + 3-section loft succeeding. Unlike the + // crash/mismatch tests above, this doesn't depend on patch 0027 at all — the exemption itself + // is unmodified pre-existing OCCT behavior — so it isn't gated on OCCTSWIFT_LOCAL. + @Test func punctualApexWithMatchingSectionsStillSucceedsUnderCreateSmoothed() throws { + let apex = try #require(Shape.vertex(at: SIMD3(0, 0, 0))) + let w1 = try #require(Wire.circle(origin: SIMD3(0, 0, 10), normal: SIMD3(0, 0, 1), radius: 4)) + let w2 = try #require(Wire.circle(origin: SIMD3(0, 0, 20), normal: SIMD3(0, 0, 1), radius: 3)) + let s1 = try #require(Shape.fromWire(w1)) + let s2 = try #require(Shape.fromWire(w2)) + let loft = ThruSectionsBuilder(isSolid: true, isRuled: false) + loft.checkCompatibility(false) + loft.addVertex(apex) + loft.addWire(s1) + loft.addWire(s2) + #expect(loft.build()) + #expect(loft.shape != nil) + } } // MARK: - CellsBuilder From 1c9545ef3a1c2bc86fa8b0e5525b356b81f3b751 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Sun, 16 Aug 2026 16:50:41 +1000 Subject: [PATCH 3/3] docs: transcribe #913 CHANGELOG entry (PR #915) Per okf/policies/changelog-on-merge.md: transcribed verbatim from the PR body as the last commit before merging. Co-Authored-By: Claude Sonnet 5 --- docs/CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 7c6dc1e3d..753d13601 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -19,6 +19,21 @@ each named with its migration in [`SEMVER.md`](SEMVER.md#v200). ## Unreleased +### `ThruSectionsBuilder` no longer returns wrong or crashing results for a mismatched section edge count under `checkCompatibility(false)` (#913) + +`BRepOffsetAPI_ThruSections::CreateSmoothed()` derived the edge count it assumes every section has +from section 1 alone, and filled a fixed-size array on that assumption with no bounds check. With +`checkCompatibility(false)`, nothing reconciles differing section edge counts first (the default, +`checkCompatibility(true)`, does this via `BRepFill_CompatibleWires`). A later section with **more** +edges than section 1 overran the array — heap corruption, observed as a SIGSEGV/SIGBUS once at +least 3 sections are involved (2 sections always take a different code path that doesn't share this +allocation shape). A section with **fewer** edges didn't crash, but silently misaligned per-section +strides to the wrong geometry, reporting `build() == true` for an invalid result. Both directions +are the same contract violation and are now rejected the same way. Fixed upstream: +[OCCT#1466](https://github.com/Open-Cascade-SAS/OCCT/pull/1466), carried as +`Scripts/patches/0027-*` and verified against a full local kernel rebuild (all 17 carried patches, +full `swift test` passing). + ### `ThruSectionsBuilder` no longer returns stale results after a failed rebuild, or after a build on a changed builder that hasn't been rebuilt (#910) `generatedFace(from:)` and `shape` both read post-build OCCT state without reliably checking