From fa9998438dd2340190813c3c5f60798c07e4fe5c Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Thu, 6 Aug 2026 09:26:33 +1000 Subject: [PATCH 1/2] chore(#705): carry the upstream ChFi2d_Builder::AddChamfer kernel patch The chamfer2D SIGSEGV fixed bridge-side in a prior PR is an upstream OCCT defect. ChFi2d_Builder::AddChamfer(E1, E2, D1, D2) calls ChFi2d::FindConnectedEdges to look up the pair's shared vertex and dereferences the two edges it returns without checking the returned status first. FindConnectedEdges leaves both edges null on every failure path, and the pair's second call fails it, because the shared vertex was already consumed chamfering the pair the first time. The sibling overload, AddChamfer(edge, vertex, distance, angle), checks the identical status correctly, and this patch makes the two-edge overload match it, four lines, reusing the null edge the function already returns on its other refusal paths. Two checks done before writing the patch: - How the five guarded call sites of FindConnectedEdges in this file return: each declares a default-constructed null value early and returns it verbatim on ChFi2d_ConnexionError. AddChamfer(E1, E2, D1, D2) already has such a variable, chamfer, and already uses it as its own declined signal on three earlier paths in the same function, so the patch returns that value rather than introducing a new one. - Whether OCCT's own tooling can reach the unguarded path: yes. BRepTest_Fillet2DCommands.cxx's chfi2d DRAW command loops over edge-name pairs from the command line and calls this same overload once per pair, so naming the same two edges twice in one invocation reaches the identical crash. Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch, verified with a debug single-TU override-link against the pinned v2.0.0-kernel.1 binary (no full rebuild): the standalone repro crashes, exit 139, on the second AddChamfer call against the stock TU, and completes cleanly, exit 0, with the patched TU linked before the OCCT archive, returning a null edge with Status() == ChFi2d_ConnexionError. clang-format reports only pre-existing, unrelated violations elsewhere in the file. Not rebuilding the xcframework or bumping the pin in this PR. The patch is carried and inert until a rebuild ships it; the bridge guard from the prior PR is what protects callers until then, per this repo's established bridge-mitigation-then-kernel-patch pattern (#298, #341, #344, #349). Filed upstream: Open-Cascade-SAS/OCCT#1431 (repro), OCCT#1432 (fix). Refs #705 --- ...AddChamfer-connexion-error-check-705.patch | 15 ++ Scripts/patches/README.md | 46 ++++++ .../705-chamfer2d-duplicate-pair/README.md | 141 ++++++++++++++++++ .../occt_705_chamfer_dup.mm | 65 ++++++++ 4 files changed, 267 insertions(+) create mode 100644 Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch create mode 100644 Scripts/repro/705-chamfer2d-duplicate-pair/README.md create mode 100644 Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm diff --git a/Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch b/Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch new file mode 100644 index 00000000..73523c84 --- /dev/null +++ b/Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch @@ -0,0 +1,15 @@ +diff --git a/src/ModelingAlgorithms/TKFillet/ChFi2d/ChFi2d_Builder_0.cxx b/src/ModelingAlgorithms/TKFillet/ChFi2d/ChFi2d_Builder_0.cxx +index 3ba6b22d..b52041c2 100644 +--- a/src/ModelingAlgorithms/TKFillet/ChFi2d/ChFi2d_Builder_0.cxx ++++ b/src/ModelingAlgorithms/TKFillet/ChFi2d/ChFi2d_Builder_0.cxx +@@ -99,6 +99,10 @@ TopoDS_Edge ChFi2d_Builder::AddChamfer(const TopoDS_Edge& E1, + // on + TopoDS_Edge EE1, EE2; + status = ChFi2d::FindConnectedEdges(newFace, commonVertex, EE1, EE2); ++ if (status == ChFi2d_ConnexionError) ++ { ++ return chamfer; ++ } + if (EE1.IsSame(E2)) + { + TopAbs_Orientation orient = EE1.Orientation(); diff --git a/Scripts/patches/README.md b/Scripts/patches/README.md index ebd6b554..b94d5a2e 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -308,6 +308,52 @@ See [`Scripts/repro/603-single-span-quadrature/`](https://github.com/SecondMouse **Retire** once the bundled OCCT includes this fix. +## 0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch + +**Fixes the upstream OCCT defect behind [#705](https://github.com/SecondMouseAU/OCCTSwift/issues/705)**, +which OCCTSwift already guards bridge-side (`OCCTFace2DChamfer`, `OCCTBridge_Modeling.mm`): +`Shape.chamfer2D(edgePairs:distances:)` SIGSEGVs, uncatchably, when the same edge pair is named +twice, found by Cluster B's fillet/chamfer edge-set census (#665). + +`ChFi2d_Builder::AddChamfer(E1, E2, D1, D2)` calls `ChFi2d::FindConnectedEdges` to look up the +pair's shared vertex, then dereferences the two edges it returns without checking the returned +status first. `FindConnectedEdges` leaves both edges null on every failure path, and a second call +naming the same pair hits exactly that: the pair's shared vertex is removed from the face's wire by +the first call's own `BuildNewWire`, so the second call's lookup fails and the two null edges it +returns reach `ComputeChamfer` unchecked. Confirmed with a debug (`-O0`) single-TU override-link +(this file compiled standalone and linked before the OCCT static archive, so the linker resolves +this TU's symbols from the override): the crash is inside `ComputeChamfer`, with `EE1`/`EE2` both +null, exit 139 on stock. + +`ChFi2d_Builder::AddChamfer(E, V, D, Ang)`, the sibling overload calling the identical +`FindConnectedEdges`, already checks the returned status correctly and returns a null edge on +`ChFi2d_ConnexionError`. Reachable from OCCT's own DRAW `chfi2d` command too +(`BRepTest_Fillet2DCommands.cxx`), which loops over edge-name pairs from the command line and calls +this same overload once per pair, so a `chfi2d` invocation naming the same two edges twice reaches +the identical crash. + +**Fix:** adds the same status check immediately after `FindConnectedEdges`, returning `chamfer`, +the default-constructed null edge this function already returns on its other refusal paths (lines +83, 89, 95), rather than a new value. + +**Validation** (override-link, no full rebuild, see the `#0001` entry above for the technique): a +rectangular planar face, `BRepFilletAPI_MakeFillet2d::AddChamfer` called twice with the identical +edge pair. Before the patch, the second call SIGSEGVs (exit 139) every time; after, it returns a +null edge with `Status() == ChFi2d_ConnexionError` (numeric 7), matching the sibling overload's own +answer for an unconnected vertex. The first call is unaffected in both cases: +`Status() == ChFi2d_IsDone` (numeric 5), a valid non-null edge. `clang-format --dry-run --Werror` +reports only pre-existing, unrelated violations elsewhere in the file, unchanged in count and +content; the four added lines are clean. + +**Bridge guard stays regardless.** This is the established pattern here (#298, #341, #344, #349): +the bridge-side duplicate-pair check in `OCCTFace2DChamfer` shipped first and is not removed by this +patch, since a caller on the currently-pinned kernel (which does not carry this patch until a +rebuild ships it) still needs it. See [`Scripts/repro/705-chamfer2d-duplicate-pair/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/705-chamfer2d-duplicate-pair) +for the reproducers. Filed upstream as [Open-Cascade-SAS/OCCT#1431](https://github.com/Open-Cascade-SAS/OCCT/issues/1431) +(repro) / [OCCT#1432](https://github.com/Open-Cascade-SAS/OCCT/pull/1432) (fix). + +**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/705-chamfer2d-duplicate-pair/README.md b/Scripts/repro/705-chamfer2d-duplicate-pair/README.md new file mode 100644 index 00000000..468f5094 --- /dev/null +++ b/Scripts/repro/705-chamfer2d-duplicate-pair/README.md @@ -0,0 +1,141 @@ +# OCCTSwift#705 reproducer, `ChFi2d_Builder::AddChamfer` null-edge SIGSEGV on a repeated pair + +Standalone, deterministic kernel-level reproducer for the uncatchable SIGSEGV in +`Shape.chamfer2D(edgePairs:distances:)` found by Cluster B's fillet/chamfer edge-set census (#665, +`Scripts/repro/cluster-b-fillet-edge-contract/`) and tracked as #705. This directory is the +kernel-level root cause and the carried patch's own evidence, separate from the bridge-side fix +(`OCCTFace2DChamfer`, `OCCTBridge_Modeling.mm`, shipped first per this repo's established +bridge-mitigation-then-kernel-patch pattern, e.g. #298/#341/#344/#349). + +## Root cause + +`ChFi2d_Builder::AddChamfer(const TopoDS_Edge& E1, const TopoDS_Edge& E2, double D1, double D2)` +(`ChFi2d_Builder_0.cxx`) calls `ChFi2d::FindConnectedEdges(newFace, commonVertex, EE1, EE2)` to look +up the two edges connected to the pair's shared vertex, then dereferences the two output edges +(`EE1.IsSame(E2)`, and both are passed into `ComputeChamfer` two lines later) without checking the +returned status first: + +```cpp +TopoDS_Edge EE1, EE2; +status = ChFi2d::FindConnectedEdges(newFace, commonVertex, EE1, EE2); +if (EE1.IsSame(E2)) // no status check first +``` + +`ChFi2d::FindConnectedEdges` (`ChFi2d.cxx`) leaves both output edges null (default-constructed) on +every failure path, including the one this repro hits: the vertex is not present in the current +face's vertex-to-edge map at all, which returns `ChFi2d_ConnexionError` with neither edge assigned. + +Calling `AddChamfer(E1, E2, D1, D2)` a second time with the *same* pair triggers exactly this: the +first call's own `BuildNewWire` rebuilds the face's wire, replacing the pair's shared vertex with +new ones where the chamfer edge meets the two trimmed edges. The second call's `CommonVertex(E1, E2, +commonVertex)` still finds the *original* vertex (it only looks at `E1`/`E2` themselves, unchanged +references), but that vertex is no longer part of the rebuilt face, so `FindConnectedEdges` fails +and the two null edges it leaves behind reach `ComputeChamfer` unchecked. + +`ChFi2d_Builder::AddChamfer(const TopoDS_Edge& E, const TopoDS_Vertex& V, double D, double Ang)`, +the sibling overload calling the identical `FindConnectedEdges`, already checks the status +correctly: + +```cpp +status = ChFi2d::FindConnectedEdges(newFace, V, adjEdge1, adjEdge2); +if (status == ChFi2d_ConnexionError) +{ + return aChamfer; +} +``` + +Confirmed via a debug (`-O0`) single-TU override-link (`ChFi2d_Builder_0.cxx` compiled standalone +and linked *before* `libOCCT-macos.a`, so the linker resolves this TU's symbols from the override, +not the stock archive member) plus the probe's own print statements: the crash happens inside +`ComputeChamfer`, called with both `EE1`/`EE2` null. + +**Reachable from OCCT's own tooling, not just this bridge.** OCCT's DRAW `chfi2d` command +(`BRepTest_Fillet2DCommands.cxx`) loops over edge-name arguments from the command line and calls +this same two-edge `AddChamfer` overload once per pair, so `chfi2d result face e1 e2 CD 1 1 e1 e2 CD +1 1` (naming the same two edges twice in one invocation) reaches the identical crash. + +## Fix + +`Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch`: adds the same +status check immediately after `FindConnectedEdges`, returning `chamfer`, the default-constructed +null edge this function already returns on its other refusal paths (`ChFi2d_Builder_0.cxx` lines +83, 89, 95), rather than a new value. Four lines, matching the sibling overload's own idiom line for +line. + +## Reproducer + +```bash +clang++ -std=c++17 -ObjC++ -w \ + -I"Libraries/OCCT.xcframework/macos-arm64/Headers" \ + -L"Libraries/OCCT.xcframework/macos-arm64" \ + Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm -o /tmp/occt_705_stock \ + -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ +/tmp/occt_705_stock +``` + +Against the pinned `v2.0.0-kernel.1` (`V8_0_1` + the eleven carried patches before this one): + +``` +edge count: 4 +about to call AddChamfer the FIRST time on edges (0,1) +first call returned, IsNull=0, status=5 +about to call AddChamfer the SECOND time on the SAME pair (0,1) +Segmentation fault: 11 +``` + +raw exit code 139, deterministic, every run. + +### Verifying the patch + +Override-link `ChFi2d_Builder_0.cxx` with the patch applied, compiled standalone with the same +`-DNo_Exception` the production kernel is built with, and link it *before* `-lOCCT-macos`: + +```bash +clang++ -c -std=gnu++17 -O0 -g -w -DNDEBUG -DNo_Exception -DOCC_CONVERT_SIGNALS \ + -I"Libraries/OCCT.xcframework/macos-arm64/Headers" \ + ChFi2d_Builder_0_patched.cxx -o ChFi2d_Builder_0_patched.o +clang++ -std=c++17 -ObjC++ -w \ + -I"Libraries/OCCT.xcframework/macos-arm64/Headers" \ + -L"Libraries/OCCT.xcframework/macos-arm64" \ + Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm ChFi2d_Builder_0_patched.o \ + -o /tmp/occt_705_patched \ + -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ +/tmp/occt_705_patched +``` + +``` +edge count: 4 +about to call AddChamfer the FIRST time on edges (0,1) +first call returned, IsNull=0, status=5 +about to call AddChamfer the SECOND time on the SAME pair (0,1) +second call returned, IsNull=1, status=7 +PROBE: completed without crashing +``` + +exit 0. `status=5` is `ChFi2d_IsDone` (the first call succeeds normally), `status=7` is +`ChFi2d_ConnexionError` (the second call now reports the same refusal the sibling overload already +gives for an unconnected vertex, instead of crashing). The first call's own result is byte-identical +before and after the patch: same `IsNull`, same `status`. + +`clang-format --dry-run --Werror` on the patched file reports only pre-existing, unrelated +violations elsewhere in the file (`Geom2dInt_GInter` construction, shifted by 4 lines, same count +and content before and after this patch); the four added lines are clean. + +## Reachable from this wrapper + +`Shape.chamfer2D(edgePairs:distances:)` (`Sources/OCCTSwift/Shape+Geom2d.swift`, bridge +`OCCTFace2DChamfer` in `Sources/OCCTBridge/src/OCCTBridge_Modeling.mm`) is the one Swift entry point +that reaches `BRepFilletAPI_MakeFillet2d::AddChamfer(edge, edge, ...)` with caller-controlled edge +pairs. The bridge now guards the duplicate-pair case itself (shipped ahead of this kernel patch, +same PR1-then-PR2 pattern as #298/#341/#344/#349), so this defect is not reachable through the +current bridge; this reproducer isolates the kernel-level mechanism the guard exists to protect +against, and is what the guard becomes redundant against once this patch ships in a release. + +`fillet2D(vertexIndices:radii:)`, the sibling entry point on the same `BRepFilletAPI_MakeFillet2d` +builder, does not reach this code path at all: a duplicated vertex index calls `AddFillet` twice, +which fails at `Build()`/`IsDone()` rather than through `FindConnectedEdges`. + +## Upstream + +Filed as [Open-Cascade-SAS/OCCT#1431](https://github.com/Open-Cascade-SAS/OCCT/issues/1431) (repro) +/ [OCCT#1432](https://github.com/Open-Cascade-SAS/OCCT/pull/1432) (fix, this patch). diff --git a/Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm b/Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm new file mode 100644 index 00000000..0f632b87 --- /dev/null +++ b/Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm @@ -0,0 +1,65 @@ +// OCCTSwift#705 kernel-level reproducer: BRepFilletAPI_MakeFillet2d::AddChamfer(E1, E2, D1, D2) +// SIGSEGVs on the second call naming the same edge pair. Bypasses OCCTBridge entirely: exercises +// the OCCT C++ API this bridge wraps, directly, so the result speaks to the upstream defect on +// its own terms, independent of anything the bridge guard now does. +// +// Compile and run against the pinned kernel: +// +// clang++ -std=c++17 -ObjC++ -w \ +// -I"Libraries/OCCT.xcframework/macos-arm64/Headers" \ +// -L"Libraries/OCCT.xcframework/macos-arm64" \ +// Scripts/repro/705-chamfer2d-duplicate-pair/occt_705_chamfer_dup.mm -o /tmp/occt_705_stock \ +// -lOCCT-macos -framework Foundation -framework AppKit -lz -lc++ +// /tmp/occt_705_stock +// +// Crashes (SIGSEGV, exit 139) on the second AddChamfer call against the stock kernel. See this +// directory's README.md for the override-link "after" verification against +// Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +int main() { + BRepBuilderAPI_MakePolygon poly; + poly.Add(gp_Pnt(0, 0, 0)); + poly.Add(gp_Pnt(10, 0, 0)); + poly.Add(gp_Pnt(10, 10, 0)); + poly.Add(gp_Pnt(0, 10, 0)); + poly.Close(); + BRepBuilderAPI_MakeFace faceMaker(poly.Wire()); + TopoDS_Face face = faceMaker.Face(); + + std::vector edges; + for (TopExp_Explorer ex(face, TopAbs_EDGE); ex.More(); ex.Next()) { + edges.push_back(TopoDS::Edge(ex.Current())); + } + printf("edge count: %zu\n", edges.size()); + if (edges.size() < 2) { + printf("FAIL: fewer than 2 edges\n"); + return 2; + } + + BRepFilletAPI_MakeFillet2d chamfer(face); + printf("about to call AddChamfer the FIRST time on edges (0,1)\n"); + fflush(stdout); + TopoDS_Edge c1 = chamfer.AddChamfer(edges[0], edges[1], 1.0, 2.0); + printf("first call returned, IsNull=%d, status=%d\n", c1.IsNull(), (int)chamfer.Status()); + fflush(stdout); + + printf("about to call AddChamfer the SECOND time on the SAME pair (0,1)\n"); + fflush(stdout); + TopoDS_Edge c2 = chamfer.AddChamfer(edges[0], edges[1], 1.0, 2.0); + printf("second call returned, IsNull=%d, status=%d\n", c2.IsNull(), (int)chamfer.Status()); + fflush(stdout); + + printf("PROBE: completed without crashing\n"); + return 0; +} From 965eb8aa84f387cb7b2350e52c8b28a28e7255c9 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:03:11 +1000 Subject: [PATCH 2/2] docs(#705): FindConnectedEdges does not null both edges on every failure Review finding, and it corrects my own analysis as well as the PR's. Both READMEs said FindConnectedEdges "leaves both edges null on every failure path". Reading the four returns, that is true of two of them and false of the other two: exactly one incident edge assigns E1 before returning ChFi2d_ConnexionError, and three or more assign both. The patch is unaffected, because it keys on the status, which IS uniform across all four. But the stated rationale was the reason to guard on nullness, and a guard written that way would have missed half the failure paths. The repro README now carries the four-way table and both say the status is what the fix checks and why. Co-Authored-By: Claude Opus 5 (1M context) --- Scripts/patches/README.md | 5 ++++- .../repro/705-chamfer2d-duplicate-pair/README.md | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Scripts/patches/README.md b/Scripts/patches/README.md index b94d5a2e..5a838f63 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -317,7 +317,10 @@ twice, found by Cluster B's fillet/chamfer edge-set census (#665). `ChFi2d_Builder::AddChamfer(E1, E2, D1, D2)` calls `ChFi2d::FindConnectedEdges` to look up the pair's shared vertex, then dereferences the two edges it returns without checking the returned -status first. `FindConnectedEdges` leaves both edges null on every failure path, and a second call +status first. `FindConnectedEdges` returns `ChFi2d_ConnexionError` on every failure path, which is +what this patch checks; it does not leave both edges null on all of them (one incident edge assigns +`E1`, three or more assign both, see the repro README's table), so nullness would have been the +wrong thing to guard on. A second call naming the same pair hits exactly that: the pair's shared vertex is removed from the face's wire by the first call's own `BuildNewWire`, so the second call's lookup fails and the two null edges it returns reach `ComputeChamfer` unchecked. Confirmed with a debug (`-O0`) single-TU override-link diff --git a/Scripts/repro/705-chamfer2d-duplicate-pair/README.md b/Scripts/repro/705-chamfer2d-duplicate-pair/README.md index 468f5094..ecccb85d 100644 --- a/Scripts/repro/705-chamfer2d-duplicate-pair/README.md +++ b/Scripts/repro/705-chamfer2d-duplicate-pair/README.md @@ -21,9 +21,19 @@ status = ChFi2d::FindConnectedEdges(newFace, commonVertex, EE1, EE2); if (EE1.IsSame(E2)) // no status check first ``` -`ChFi2d::FindConnectedEdges` (`ChFi2d.cxx`) leaves both output edges null (default-constructed) on -every failure path, including the one this repro hits: the vertex is not present in the current -face's vertex-to-edge map at all, which returns `ChFi2d_ConnexionError` with neither edge assigned. +`ChFi2d::FindConnectedEdges` (`ChFi2d.cxx`) returns `ChFi2d_ConnexionError` on **every** failure +path, which is what the fix keys on. It does **not** leave both edges null on every one of them, and +the distinction matters for anyone reasoning about a different failure than this repro's: + +| failure | `E1` | `E2` | +|---|---|---| +| vertex absent from the face's vertex-to-edge map (this repro) | unassigned | unassigned | +| vertex present, zero incident edges | unassigned | unassigned | +| exactly one incident edge | **assigned** | unassigned | +| three or more incident edges | **assigned** | **assigned** | + +So a guard written against nullness would miss two of the four. The status is uniform across all +four, so that is what the patch checks. Calling `AddChamfer(E1, E2, D1, D2)` a second time with the *same* pair triggers exactly this: the first call's own `BuildNewWire` rebuilds the face's wire, replacing the pair's shared vertex with