Modeling Algorithms - ShapeUpgrade_UnifySameDomain: guard null pcurve dereferences#1392
Open
gsdali wants to merge 1 commit into
Open
Conversation
… dereferences Fixes Open-Cascade-SAS#1391. IntUnifyFaces (and its file-local SplitWire helper) disambiguate between multiple candidate next-edges at a branching vertex by comparing each candidate's pcurve tangent direction on the current reference face. Three call sites in IntUnifyFaces (and the matching pair in SplitWire) fetched that pcurve via BRep_Tool::CurveOnSurface and dereferenced it immediately without checking IsNull() first, unlike every other CurveOnSurface call site in this file. CurveOnSurface legitimately returns a null handle when an edge has no pcurve on the given face -- the common case for a raw mesh-sewn solid at a vertex shared by more than two edges -- so the dereference is a null-pointer virtual call, uncatchable in-process. Guard all five sites with IsNull() checks, following the file's own established pattern: a missing pcurve on a *candidate* edge means "skip it, not a rankable direction"; a missing pcurve on the *current* edge (so nothing to compare against) falls back to treating all candidates as equally likely, same as the existing single-candidate shortcut just above it. Verified against the attached artifact: builds successfully with the patch (multiple runs), SIGSEGVs 100% of the time without it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the change
Guards five unguarded
BRep_Tool::CurveOnSurface(...)dereferences inShapeUpgrade_UnifySameDomain::IntUnifyFacesand its file-localSplitWirehelper withIsNull()checks, following the file's own established pattern used at every otherCurveOnSurfacecall site.CurveOnSurfacereturns a nullHandle(Geom2d_Curve)when an edge has no pcurve on the givenface — routine for a raw mesh-sewn solid (
BRepBuilderAPI_Sewingoutput from an STL/mesh import)at a vertex shared by more than two edges. The five sites dereference the result immediately
(
->D1(...)/->Value(...)) with no check, which is a null-pointer virtual call — a rawSIGSEGV, uncatchable in-process (this is a memory-safety bug, not aStandard_Failurea callercould
try/catch).continue).back to treating all candidates as equally likely — the same fallback the surrounding code
already takes for the "only one candidate" case.
Related issue
Fixes #1391.
Type of change
How Has This Been Tested?
ShapeUpgrade_UnifySameDomain::Build().Debug (
-g -O0) single-TU override-link +lldb bton stockmasterresolves the crash toShapeUpgrade_UnifySameDomain.cxx:4003(aPCurve->D1(...), null handle), reached viaIntUnifyFaces→UnifyFaces→Build.clang-format --dry-run --Werroris clean on every line this patch touches (two pre-existingformatting violations elsewhere in the file, both untouched by this patch, are unrelated
pre-existing drift).
Reproducer artifact + standalone C++ driver:
https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/348-unify-null-pcurve
Checklist:
"does not crash"
wrapper project (OCCTSwift) since this repo's DRAW test suite doesn't have an equivalent
fixture-driven mesh-sew flow readily available; happy to add a DRAW test if maintainers
point me at the right harness/fixture convention for this area