Shape Healing - create a ReShape context when none is set in ShapeFix_ComposeShell and ShapeUpgrade_WireDivide - #1410
Conversation
|
Dear @gsdali please add GTest to cover your changes, you can follow the logic of all exited GTests. |
e0b16ee to
5f1e175
Compare
…_ComposeShell and ShapeUpgrade_WireDivide Fixes Open-Cascade-SAS#1409. ShapeFix_ComposeShell::Perform(), ShapeFix_ComposeShell::SplitEdges() and ShapeUpgrade_WireDivide::Perform() dereference Context() unconditionally. ShapeFix_Root::Context() returns myContext, which the base constructor leaves null and only an explicit, optional SetContext() ever fills - so Init(...) + Perform(), the usage both classes' public API invites, is a null-handle dereference (SIGSEGV at address 0, uncatchable in-process). A plain 4-edge planar square face reproduces it 100% of the time; see Open-Cascade-SAS#1409 for a self-contained reproducer. The reason this has gone unnoticed: in-kernel, both classes are only ever driven by ShapeUpgrade_FaceDivide::Perform(), which already self-creates a context and hands it down (ShapeUpgrade_FaceDivide.cxx:185 for the compose shell, :238 for the wire divide). Reached that way neither class ever sees a null context, so the existing tests never exercise the crash. Add the guard that nine other classes in the same package already use - ShapeFix_Shape::Init, ShapeFix_Shell::Perform, ShapeFix_Solid::Perform, ShapeFix_FixSmallFace::Init, ShapeFix_SplitCommonVertex::Init, ShapeFix_Wireframe (both entry points), ShapeFix_Wire::FixGap3d/FixGap2d, ShapeFix_Face::FixMissingSeam, ShapeUpgrade_ShapeDivide::Perform - to the three public entry points that were missing it. None of those nine sibling guards carries a comment, so this one doesn't either. ShapeFix_ComposeShell's other context-dereferencing methods (LoadWires, SplitWire, SplitByLine, MakeFacesOnPatch, DispatchWires) are all reached through Perform() or SplitEdges(), so guarding those two covers them; they are also const, so they could not create a context themselves. Same defect class as Open-Cascade-SAS#1378 / Open-Cascade-SAS#1380 (merged), which fixed ShapeFix_Face::FixPeriodicDegenerated. New GTests in src/ModelingAlgorithms/TKShHealing/GTests/: ShapeFix_ComposeShell_Test.cxx (Perform and SplitEdges called with no context set, plus a with/without-context equivalence check) and ShapeUpgrade_WireDivide_Test.cxx (same shape). Verified both ways: linked against an override build with the guard removed, all three *_WithoutContext_DoesNotCrash cases SIGSEGV (exit 139); linked against the guarded build, all five tests pass. Verified downstream on V8_0_0_p1 with the two patched translation units linked ahead of the stock archive, so only these symbols change: - Crash closed - the reproducer from Open-Cascade-SAS#1409, plus the same two calls against an unbounded-cylindrical face, SIGSEGV 100% of the time before and complete normally after. - No behaviour change on the working path - with a context set (the only path that worked before), the result is byte-identical before and after: BRepTools::Write dump hash plus face/wire/edge/vertex counts, for both a planar and a cylindrical face, both classes. The guard only fires when there was no context, and in that case it produces exactly what an explicitly-provided context produces. - clang-format clean on all four touched/added files.
5f1e175 to
69da097
Compare
|
Thanks — pushed an update. GTests added: Comments shortened: dropped the three explanatory comments on the new guards entirely, matching the nine sibling guards already in this file/package (e.g. |
Fixes #1409.
Problem
ShapeFix_ComposeShell::Perform(),ShapeFix_ComposeShell::SplitEdges()andShapeUpgrade_WireDivide::Perform()dereferenceContext()unconditionally.ShapeFix_Root::Context()returnsmyContext, which the base constructor leaves null and only an explicit, optionalSetContext()ever fills — soInit(...)+Perform(), the usage both classes' public API invites, is a null-handle dereference (SIGSEGV at address 0, uncatchable in-process). A plain 4-edge planar square face reproduces it 100% of the time; see #1409 for a self-contained reproducer.The reason this has gone unnoticed: in-kernel, both classes are only ever driven by
ShapeUpgrade_FaceDivide::Perform(), which already self-creates a context and hands it down (ShapeUpgrade_FaceDivide.cxx:185for the compose shell,:238for the wire divide). Reached that way neither class ever sees a null context, so the existing tests never exercise the crash.Fix
Add the guard that nine other classes in the same package already use —
ShapeFix_Shape::Init,ShapeFix_Shell::Perform,ShapeFix_Solid::Perform,ShapeFix_FixSmallFace::Init,ShapeFix_SplitCommonVertex::Init,ShapeFix_Wireframe(both entry points),ShapeFix_Wire::FixGap3d/FixGap2d,ShapeFix_Face::FixMissingSeam,ShapeUpgrade_ShapeDivide::Perform— to the three public entry points that were missing it:ShapeFix_ComposeShell's other context-dereferencing methods (LoadWires,SplitWire,SplitByLine,MakeFacesOnPatch,DispatchWires) are all reached throughPerform()orSplitEdges(), so guarding those two covers them; they are alsoconst, so they could not create a context themselves.Same defect class as #1378 / #1380 (merged), which fixed
ShapeFix_Face::FixPeriodicDegenerated.Validation
Verified downstream on
V8_0_0_p1with the two patched translation units linked ahead of the stock archive, so only these symbols change:BRepTools::Writedump hash plus face/wire/edge/vertex counts, for both a planar and a cylindrical face, both classes. The guard only fires when there was no context, and in that case it produces exactly what an explicitly-provided context produces.clang-formatclean on both files.