Skip to content

Shape Healing - guard the null Context() in ShapeFix_Face::FixPeriodicDegenerated#1380

Open
gsdali wants to merge 1 commit into
Open-Cascade-SAS:masterfrom
gsdali:fix/317-shapefix-face-null-context-periodicdegenerated
Open

Shape Healing - guard the null Context() in ShapeFix_Face::FixPeriodicDegenerated#1380
gsdali wants to merge 1 commit into
Open-Cascade-SAS:masterfrom
gsdali:fix/317-shapefix-face-null-context-periodicdegenerated

Conversation

@gsdali

@gsdali gsdali commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #1378.

Problem

ShapeFix_Face::Perform() SIGSEGVs healing a face whose sole boundary wire is a single closed edge
belting the full 2π period of a Geom_ConicalSurface (positioned so the apex sits outside the
wire's V range) — the exact case FixPeriodicDegenerated() exists to patch in a degenerate apex
edge for. Only the plain, most common usage crashes: ShapeFix_Face fixer(face); fixer.Perform();,
with no explicit SetContext() call.

Root cause

FixPeriodicDegenerated() builds the degenerate apex edge/wire, assembles a new face from the
original wire plus the apex wire, and finalizes:

myResult = aNewFace;
Context()->Replace(myFace, myResult);

Every other Context()->Replace call site in this file — eleven of them — guards against a null
Context() first (either a plain if (!Context().IsNull()), or a lazy SetContext(new ShapeBuild_ReShape) when null). FixPeriodicDegenerated even checks Context().IsNull() at the
top of the function before calling Context()->Apply() — but the check is missing at the
bottom. Context() returns ShapeFix_Root::myContext, left null by the base constructor and set
only by an explicit SetContext() call — which the ordinary construction ShapeFix_Face fixer(face); fixer.Perform(); never makes. Any caller healing a lone periodic-conical wire this way
null-derefs.

Fix

The same guard used at every other call site in the file — only replace in the context if there is
one to replace in.

Reproducer / test

TColgp_Array1OfPnt pts(1, 8);
const double r = 0.14;
for (int i = 1; i <= 8; i++) {
    double a = 2.0 * M_PI * (i - 1) / 8;
    pts.SetValue(i, gp_Pnt(r * cos(a), 4.0 + r * sin(a), -9.6));
}
Handle(TColgp_HArray1OfPnt) hpts = new TColgp_HArray1OfPnt(1, pts.Length());
for (int i = pts.Lower(); i <= pts.Upper(); i++) hpts->SetValue(i, pts.Value(i));

GeomAPI_Interpolate interp(hpts, Standard_True, 1e-6);
interp.Perform();

BRepBuilderAPI_MakeEdge me(interp.Curve());
BRepLib_MakeWire mw;
mw.Add(me.Edge());

gp_Ax3 axis(gp_Pnt(0.0, 4.0, -9.1), gp_Dir(0.0, 0.09, -1.0));
Handle(Geom_ConicalSurface) cone = new Geom_ConicalSurface(axis, 0.35, 0.0);
BRepBuilderAPI_MakeFace mf(cone, mw.Wire(), Standard_True);

ShapeFix_Face fixer(mf.Face());  // crashes on master
fixer.Perform();

Crashes on current master; fixed here. Added GTest
ShapeFix_FaceTest.NoCrashOnPeriodicConicalSingleWire.

Validation

Diagnosed with a custom backtrace_symbols_fd SIGSEGV handler (lldb/core dumps unavailable in
the diagnosing sandbox), pinpointing the crash to ShapeFix_Face::FixPeriodicDegenerated; an -O0
single-TU override-link (compiling just the patched .cxx and linking it before the archive)
confirmed every statement in the function up to the final Context()->Replace completes normally,
and that call is specifically where it faults. Both the minimal 8-point synthetic reproducer above
and a 10-point closed curve fit through real mesh-derived rivet-rim points (the case this was
originally surfaced from) crash 100% of the time on stock V8_0_0_p1 and survive with this patch,
returning a valid healed face (BRepCheck_Analyzer::IsValid() == true).

Reported downstream and isolated at SecondMouseAU/OCCTSwift#317.

…cDegenerated

ShapeFix_Face::Perform SIGSEGVs healing a face whose sole boundary wire is a
single closed edge belting the full period of a Geom_ConicalSurface, positioned
so the apex sits outside the wire's V range. FixPeriodicDegenerated finalizes
with an unconditional Context()->Replace(myFace, myResult); every other
Context()->Replace call site in this file guards Context().IsNull() first (the
function's own top even does, for Context()->Apply()) -- this one didn't, so a
bare "ShapeFix_Face fixer(face); fixer.Perform();" (the ordinary usage, no
SetContext call) null-derefs. Restores the same guard used everywhere else in
the file.

Added GTests: NoCrashOnPeriodicConicalSingleWire (crashes on master, survives
with this patch) and ValidFaceOnPeriodicConicalSingleWireWithContext (same
input healed with a context produces a genuinely valid face -- the guard only
skips recording a replacement that has nowhere to go, it does not skip
building the degenerate apex edge itself).

Reported downstream and isolated at SecondMouseAU/OCCTSwift#317.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

ShapeFix_Face::FixPeriodicDegenerated() SIGSEGVs on a null Context() (unguarded Context()->Replace)

1 participant