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..5a838f63 100644 --- a/Scripts/patches/README.md +++ b/Scripts/patches/README.md @@ -308,6 +308,55 @@ 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` 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 +(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..ecccb85d --- /dev/null +++ b/Scripts/repro/705-chamfer2d-duplicate-pair/README.md @@ -0,0 +1,151 @@ +# 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`) 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 +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; +}