Skip to content

Do not build a PickedPoint from a pick that addresses nothing - #6772

Merged
Fedr merged 2 commits into
masterfrom
fedr/guard-picked-point
Sep 4, 2026
Merged

Do not build a PickedPoint from a pick that addresses nothing#6772
Fedr merged 2 commits into
masterfrom
fedr/guard-picked-point

Conversation

@Fedr

@Fedr Fedr commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

The crash

MeshInspector's create_feature_sphere UI test died with SIGSEGV on the Ubuntu 26.04 leg of MeshInspectorCode#7746, on the mouse-down pick right after the BestFit Sphere activation:

[info] pressButton Ribbon/BestFit Sphere: frame 24
[info] Activated item: "BestFit Sphere"
[critical] Crash signal: 11
 3# MR::toTriPoint(MeshTopology const&, VertCoords const&, FaceId, Vector3f const&)
 4# MR::pointOnObjectToPickedPoint(VisualObject const*, PointOnObject const&)
 5# MR::PickPointManager::onMouseDown_(MouseButton, int)

pointOnObjectToPickedPoint() fed pos.face straight into Mesh::toTriPoint(), which calls topology.edgeWithLeft( f ) — an unchecked edgePerFace_[f]. So a pick carrying a face that the topology does not have is an out-of-bounds read, and a null mesh() is a null dereference. The sibling function directly below, getPickedPointPosition(), already validates with topology.hasEdge(...) before touching anything; this one did not.

The fix

Validate the address against the object before converting, in all three branches:

  • mesh: mesh() non-null, pos.face valid and topology.hasFace( pos.face )
  • points: pointCloud() non-null, pos.vert valid and in validPoints
  • lines: polyline() non-null, the edge valid and topology.hasEdge(...)

and otherwise return std::monostate, which MRPointOnObject.h already documents as "means not valid pick (pick in empty space)" — so this is the variant's existing vocabulary, not a new convention. The header comment now says so on the function too.

The spdlog::warn naming the offending id is deliberate: a pick that addresses nothing should be visible in the log rather than silently dropped, and it is what will tell us whether the 26.04 pick really is bogus (see below).

PickPointManager::onMouseDown_() passed the conversion result to appendPoint() unchecked, which would now append a monostate point; it refuses instead.

Why only Ubuntu 26.04, and what is still open

The 22.04 (Clang) and 24.04 (GCC 14) legs pass this same UI test in the same run. 26.04 runs a much newer software GL stack under xvfb — llvmpipe (LLVM 21.1.8, 256 bits), Mesa 26.0.8 — and the pick is read back from a rendered buffer, so a different renderer plausibly yields a different (here: unusable) pick result.

That is a hypothesis, not a proven root cause, and this PR does not claim to fix it: it converts an out-of-bounds read into a rejected pick and a log line. If the warning fires on 26.04 with an out-of-range face, the pick is confirmed bogus and the remaining question is why the renderer produces it. If the crash were to persist without the warning, the input was valid and the fault lies elsewhere — equally worth knowing.

Guarding is correct regardless of the answer: a pick arriving from a GPU read-back is untrusted input.

Verification

MRMesh.vcxproj and MRViewer.vcxproj build clean locally (Release x64, 0 warnings). The instrumented run on the 26.04 leg is what settles the pick question, and I will report what the log says.

pointOnObjectToPickedPoint() fed pos.face straight into
Mesh::toTriPoint(), which indexes edgePerFace_ by it, so a pick carrying
an out-of-range face read out of bounds. That is a live crash: the
MeshInspector UI test create_feature_sphere died with SIGSEGV inside
toTriPoint() under PickPointManager::onMouseDown_ on Ubuntu 26.04, whose
much newer llvmpipe (LLVM 21.1.8, Mesa 26.0.8) evidently returns a
different pick than the 22.04 and 24.04 legs, which pass.

Validate the address against the object before converting - the face
against the topology, the vertex against validPoints, the edge against
the polyline - and return std::monostate, which the header already
documents as "not valid pick". The warning names the offending id so a
bogus pick is visible in the log instead of being silently dropped.

PickPointManager::onMouseDown_ then refuses to append such a point; it
used to pass the conversion result on unchecked.
@Fedr
Fedr merged commit e8487c5 into master Sep 4, 2026
39 checks passed
@Fedr
Fedr deleted the fedr/guard-picked-point branch September 4, 2026 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants