Skip to content

fix: an offset refuses an effectively zero approximation tolerance; the nightly's C++ crash is UB-291 (#2393) - #2733

Merged
bherbruck merged 2 commits into
tangency-labs:mainfrom
aapolipponen:fix/2393-offset-crash
Nov 5, 2026
Merged

bherbruck merged 2 commits into
tangency-labs:mainfrom
aapolipponen:fix/2393-offset-crash

Conversation

@aapolipponen

@aapolipponen aapolipponen commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Units: offset.offset_executive_p2

Fixes the nightly fuzz's C++ crash, fuzz00167 (run 41821585108, 2026-11-04), part of #2393. It builds on #2737, now merged, which made such crashes rejected cases and covers the other failures from that night.

The crash: UB-291 (tangency-labs/smlib#135)

An extended offset with an approximation tolerance of −1e-300 crashes the C++ in SmBrep::ReplaceSurface (SmBrep.cpp:26589), called from SmOffsetExecutive::DoExtendedOffset. Each step below was confirmed in gdb:

  1. SmZoneTol3d takes fabs, so the offset B-rep's tolerance becomes 1e-300 (SmOffsetExecutive.cpp:1115). SetTolerance's automatic fallback only catches ±0.
  2. At that tolerance FindInfiniteRegion finds no region and returns 1006. MakeFaceTopology fails, and so does MakeFace.
  3. MakeFace still resets the sphere's owner to null (:19158), leaving the half-built face in the B-rep.
  4. BuildOffsetOfFaceuse ignores the failure (:3753, no else branch).
  5. The extension loop (:1683) calls ReplaceSurface on that face, and pOld->GetFace() is null.

Recorded as plan/upstream-issues/UB-291--…. fuzz00167 is a fixture and is parked under UB-291; #2737's child-process trap makes the crash a rejected case.

The Rust side was wrong too: WR-749

On the same input the Rust side returned Ok with the right sphere, radius 2.9, but in a B-rep of tolerance 1e-300. That body fails the kernel's own check_brep:

  • "Tolerance less than effective zero";
  • edge and vertex gaps of 7.7e-15 above that tolerance.

Fix. check_resolvable, the guard #2737 added, now also refuses a nonzero approximation tolerance at or below EFF_ZERO (1e-12). The refusal is OffsetFailure::ToleranceUnresolved { tolerance }, a structured failure of kind InvalidInput that names the value. An exact zero still takes upstream's automatic tolerance. The check runs before anything is built, for the static, solid and extended offsets. The C++ result is recorded as WR-749.

Results that change

Case Before After Why the new result is right
fuzz00167 (approx −1e-300) C++ crash; Rust Ok, tolerance 1e-300 ToleranceUnresolved, parked UB-291 the body fails the kernel's check 0
tiny_tolerance_sheet_exact_boundary, _above_boundary pass: both sides shell a sheet of tolerance 1e-15 into a result of tolerance 1e-15 refused, parked WR-749, ratchet waiver upstream passes the source's tolerance on as the approximation tolerance; gdb shows SetTolerance(1e-15), below SM_EFF_ZERO, so the result fails check 0
_below_boundary the C++ refuses with 1007 unchanged

#2737's test. I changed the second half of #2737's fuzz00087 test. It asserted that an approximation tolerance of −1e-300 builds a body of tolerance 1e-300, which is the result this PR replaces. It now expects ToleranceUnresolved, and is renamed fuzz00087_is_refused_by_its_distance_then_by_its_approximation_tolerance.

The two differs from that night (fuzz00030, distance −0.0; fuzz00160, distance 1e-300) are covered by #2737's distance refusal: both are rejected on this base.

Evidence

  • New test issue_2393_extended_offset_tiny_approx_tol, 2 passed:
    • the refusal, which fails without the fix: the old code returned Ok;
    • the fixture's normal tolerance still builds the sphere, checked against its closed-form volume.
  • offset_negative_approximation_tolerance: 4 passed.
  • --unit offset.offset_executive_p2: PASS, 594 cases: 584 passed, 0 failed, 6 rejected, 4 parked (UB-241, UB-291, WR-749 ×2).
  • --fuzz 300: FUZZ-PASS, 0 crashes. The 5 differs all have an approximation or source tolerance of ±1e-300, where the C++ builds and the Rust side refuses: WR-749 inputs.
  • fmt, clippy, the wasm check and check_record_ids.py are clean.

Related: #2393, #2737, tangency-labs/smlib#135.
Status: ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Offset operations now return a clear invalid-input error when given a nonzero approximation tolerance at or below the effective-zero threshold, rather than proceeding with an unresolved tolerance.
    • An exact zero approximation tolerance remains accepted.
    • Extended offsets with resolvable tolerances continue to support valid results, including manifold solids.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The offset executive now rejects nonzero approximation tolerances at or below EFF_ZERO with a typed invalid-input error. Regression tests and difftest records cover tiny tolerances, boundary cases, and an upstream crash case.

Changes: Offset tolerance validation

Layer / File(s) Summary
Tolerance refusal and error contract
crates/solidcore/src/error/offset.rs, crates/solidcore/src/offset/offset_executive/p2.rs
check_resolvable rejects nonzero approximation tolerances whose absolute value is at or below EFF_ZERO. The new ToleranceUnresolved error maps to InvalidInput and SM_ERR_INVALID_INPUT; zero tolerance remains accepted.
Offset tolerance regression coverage
crates/solidcore/tests/issue_2393_extended_offset_tiny_approx_tol.rs, crates/solidcore/tests/offset_negative_approximation_tolerance.rs
Tests check refusals for tiny tolerances, including negative values and the effective-zero boundary. They also check sequential distance and tolerance refusals, and a successful ordinary-tolerance sphere offset.
Upstream mismatch records and waivers
fixtures/offset.offset_executive_p2/fuzz00167.json, crates/difftest/known_mismatches/offset.offset_executive_p2.toml, plan/difftest-waivers.toml, plan/upstream-issues/*
The fixture and issue reports document the upstream crash case. Mismatch records and waivers document the crash and tiny-tolerance sheet cases alongside solidcore’s refusals.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related issues

  • Issue 1018: Defines safe Rust behavior when SMLib crashes or has undefined behavior; this change rejects tolerances associated with an upstream crash.
  • Issue 2615: Requests typed, descriptive solidcore errors; this change adds OffsetFailure::ToleranceUnresolved with an InvalidInput kind.

Possibly related PRs

Suggested reviewers: jyviko, bherbruck

Merge Risk: Minimal · up to 58c04

Tiny nonzero tolerances are rejected while zero retains automatic tolerance. No merge-blocking risk was identified.

Security architecture risk: Minimal · up to 58c04

The change rejects unusably small nonzero tolerances before constructing an offset result. The new error preserves the supplied value and uses the existing invalid-input classification. No material security risk introduced or worsened by this change was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated effect is bounded to offset calls using a nonzero approximation tolerance at or below the threshold after distance validation succeeds. Such calls now receive a refusal rather than entering result construction. The inspected evidence does not establish remote reachability, tenant exposure, or service-level privileges.

Trust Boundaries and Controls

  • observed — The new control checks the caller-configured tolerance before the allocation routine applies its magnitude to the result B-rep. Rejection returns the existing structured failure mechanism rather than granting access to construction or changing an identity or authority decision.

Resilience and Maintainability Implications

  • inferred — The added refusal occurs in a read-only precondition check before result allocation and notification-record writes. Consequently, the new rejection branch does not introduce an intermediate result, ownership transfer, or cleanup obligation; repeated rejected calls do not progress that construction transition.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the main change: offset operations now refuse nonzero approximation tolerances at or below the effective zero (1e-12). It also names the related C++ crash (UB-291) and issue #2393. The…
Docstring Coverage Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: offset operations now refuse nonzero approximation tolerances at or below the effective zero (1e-12). It also names the related C++ crash (UB-291) and issue #2393. The changed files match this: the new ToleranceUnresolved variant, the check_resolvable rule, and the regression tests all cover this refusal. The title is long, at 109 characters, and includes a second clause about the nightly C++ crash, but it still relates to the changeset and is not vague.

Full details: Docstring Coverage

Explanation

Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 4 files. (5 skipped: 5 unsupported.)

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a91e108 and 58c0416.

Files selected for processing: 9
  • crates/difftest/known_mismatches/offset.offset_executive_p2.toml
  • crates/solidcore/src/error/offset.rs
  • crates/solidcore/src/offset/offset_executive/p2.rs
  • crates/solidcore/tests/issue_2393_extended_offset_tiny_approx_tol.rs
  • crates/solidcore/tests/offset_negative_approximation_tolerance.rs
  • fixtures/offset.offset_executive_p2/fuzz00167.json
  • plan/difftest-waivers.toml
  • plan/upstream-issues/UB-291--extended-offset-replacesurface-dereferences-the-null-owner-of-a-half-built-face.md
  • plan/upstream-issues/WR-749--offset-b-rep-takes-an-effectively-zero-approximation-tolerance.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@jyviko

jyviko commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Thanks @aapolipponen, fuzz00167 is yours. A PR of ours on the same nightly (opening shortly as fix/nightly-fuzz) covers the other failure, fuzz00175 (a member distance of 1e-300 refused by the kernel), and seeds fuzz cases by fixture name so adding a fixture no longer reshuffles every case. It leaves 00167's record and parking to you and will link here. The crash is also tracked upstream as tangency-labs/smlib#135.

@aapolipponen

Copy link
Copy Markdown
Contributor Author

Thanks @jyviko. I'll cite tangency-labs/smlib#135 in the record, and park 00167 so it fits your fixture-name seeding once fix/nightly-fuzz is up.

…he nightly's C++ crash is UB-291 (tangency-labs#2393)

Nightly fuzz00167 (2026-11-04) crashed the C++: an approximation tolerance of -1e-300
becomes the offset B-rep's tolerance 1e-300 through fabs, a face build fails at that
tolerance, BuildOffsetOfFaceuse ignores the failure, and the extension loop calls
ReplaceSurface on the half-built face, whose surface has no owner (UB-291,
tangency-labs/smlib#135). The Rust side returned Ok with a body of tolerance 1e-300,
which fails the kernel's own check (test 0, tolerance below effective zero).

check_resolvable now refuses a nonzero approximation tolerance at or below EFF_ZERO:
OffsetFailure::ToleranceUnresolved, kind InvalidInput, naming the value. An exact zero
still takes upstream's automatic tolerance. The C++ result is recorded as WR-749. Two
static shells of a sheet whose own tolerance is 1e-15 now refuse where the C++ builds
an invalid body; they are parked under WR-749. fuzz00167 is a fixture, parked under
UB-291.
@aapolipponen
aapolipponen force-pushed the fix/2393-offset-crash branch from 3896a81 to 58c0416 Compare November 4, 2026 23:15
@aapolipponen aapolipponen changed the title fix: the nightly fuzz's C++ crash in offset.offset_executive_p2, fuzz00167 (#2393) fix: an offset refuses an effectively zero approximation tolerance; the nightly's C++ crash is UB-291 (#2393) Nov 4, 2026
@aapolipponen
aapolipponen marked this pull request as ready for review November 4, 2026 23:47

@bherbruck bherbruck left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by the automated first-pass review: CI green, mergeable, no CI/build/dependency file changes, no red-flag patterns in added code (process/network/env/unsafe outside FFI/blobs/hidden unicode), 0 known-mismatch entries added. Logic is gated by CI and the difftest merge queue.

@bherbruck
bherbruck merged commit 7b5a234 into tangency-labs:main Nov 5, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants