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 new file mode 100644 index 000000000..19fba527b --- /dev/null +++ b/Scripts/patches/0027-BRepOffsetAPI_ThruSections-CreateSmoothed-section-edge-count-guard-913.patch @@ -0,0 +1,153 @@ +From: OCCTSwift ecosystem +Subject: [PATCH] Modeling Algorithms - guard CreateSmoothed's fixed-stride + 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 +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 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. 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;). 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) 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; 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 | 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 +760,51 @@ void BRepOffsetAPI_ThruSections::CreateSmoothed() + } + } + ++ // #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, 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 (isPunctualSection(iSect)) ++ { ++ // 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()) ++ { ++ aSectEdges++; ++ } ++ if (aSectEdges != nbEdges) ++ { ++ myStatus = BRepFill_ThruSectionErrorStatus_ProfilesInconsistent; ++ return; ++ } ++ } ++ + // 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 c5d622661..0af10e363 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -765,6 +765,115 @@ 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 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 (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 (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`, 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). + +**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/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 fa94d7bab..9d55a97aa 100644 --- a/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift +++ b/Tests/OCCTStressTests/StressBuilderLifecycleTests.swift @@ -431,6 +431,63 @@ struct StressThruSectionsBuilderLifecycleTests { _ = 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. + // + // 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) + loft.addWire(s2) + #expect(loft.build()) + + // A third section with MORE edges (a triangle, 3) than the first two (1 each, circles). + let triangle = try #require(Wire.polygon3D([ + SIMD3(2, 0, 20), SIMD3(-1, 1.7320508, 20), SIMD3(-1, -1.7320508, 20) + ], 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) + } + // #910: a reused builder's `generatedFace(from:)` must not hand back a first, successful // build's face data once a later rebuild on the same instance has failed. OCCT's own // `GeneratedFace()` is a bare `myEdgeFace` lookup that `Build()` never clears, so the guard diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 39adf9b23..66a491795 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). + ### Code-style CI: swift-format, SwiftLint, and clang-format gates (#876) Adds automated style enforcement for new/touched code: `swift-format` (Swift formatting),