Repository navigation
fix(snapshot): restore refuses a mesh with fewer than 3 vertices - #423
Conversation
siracozmen01
left a comment
There was a problem hiding this comment.
Review at b23f4a6: changes needed (1). I read the code and did not run it.
1. The 1-vertex cut is refused on main too, for another reason, so it does not test the new check. The mesh's one triangle is {0, 1, 0}. Cut to 1 vertex, the triangle still names vertex 1. MeshContentFits (src/snapshot.c:210-217) already refuses an index at or past the vertex count, so m3World_Restore returns m3_errorFormat on main for keep == 1 as well.
- Only the 2-vertex cut fails without the new condition. The body's "With the new check removed, the test fails on the refusal" holds for that cut alone.
- Make the triangle use vertex 0 only (
{0, 0, 0}, which create takes as it takes{0, 1, 0}). Then both cuts stay valid apart from the vertex count, and each is refused only byvc > 0 && vc < 3. Please confirm both fail on main.
What you asked about.
- The same bound as create.
MeshContentCheckinsrc/shape.c:148refusesvertexCount < 3 || triangleCount < 1. The new(vc > 0 && vc < 3)with the existing(vc == 0) != (tc == 0)refuses the same live meshes, since a live mesh's triangle count is then at least 1. The 3 is a literal in both places, not a shared constant, so the two agree now but nothing ties them. An internal constant used by both would make drift impossible. That is not needed for this fix to be correct. - Only live meshes. A freed slot writes 0 and 0.
vc > 0 && vc < 3is false for it, and the pairing check still holds, so a world with destroyed meshes restores as before. - Nothing the library writes is refused, and no hash moves. Every live mesh comes from a create or a replay through
MeshContentCheck, so its vertex count is 3 or more. The change is a refusal inMeshTailEndbefore any write, and no write path changes.TestRestoreAcceptsWhatTheEngineWritesstill covers the written forms. fixwithout!fits at 1.x: restore now refuses bytes the library never writes, and no API changes.- The body has Problem, Change and Verification, and no column-0
name(line.
siracozmen01
left a comment
There was a problem hiding this comment.
Re-review at b2b7b11: ready, once CI goes green.
-
My point is fixed. For the 1-vertex cut the test now zeroes the three indices at
cut + head, whereMeshContentFitsreads them, right after the kept vertices. Both cuts pass the index check, and only the vertex count is short. The session reports that both fail with main's restore. -
The constant changes no behavior.
M3_MESH_MIN_VERTICES(3, inworld_internal.h) replaces the literal at the four sites, each with the same comparison and the same refusal:MeshInputCheckinshape_create.c(m3_errorInvalid)MeshContentCheckinshape.c(m3_errorInvalid)m3ReplayCreateMeshShapeinjournal_replay_shapes.c(false)MeshTailEndinsnapshot.c(-1)
Both files that did not have it now include
world_internal.h. The comparisons are against the sameint32_tvalues. -
No minimum-vertex literal is left in
shape_create.c,shape.c,journal_replay_shapes.c,snapshot.c,shape_data.c,shape_state.c,mesh_bvh.corjournal.c. Thek < 3loops that remain walk a triangle's corners.
No findings.
Problem
m3CreateMeshShaperefuses a mesh with fewer than 3 vertices withm3_errorInvalid, inMeshContentCheckinsrc/shape.c, and replay goes through the same check.m3World_Restorechecks a mesh's counts inMeshTailEndinsrc/snapshot.c. That check refuses only a negative count, a count past its limit, and a vertex count that is zero while the triangle count is not, or the reverse. So a snapshot could hold a mesh of 1 or 2 vertices, which no create or replay builds, and the restored world stepped with it. A probe on main cut a 3-vertex mesh to 2 and to 1 vertex in its snapshot, and both restored withm3_success(#422).Change
MeshTailEndalso refuses a live mesh with fewer thanM3_MESH_MIN_VERTICESvertices, so restore returnsm3_errorFormatbefore any byte lands and leaves the world as it was. A live mesh already has at least 1 triangle, since its triangle count is zero only when its vertex count is.M3_MESH_MIN_VERTICES, 3, is a new internal name insrc/world_internal.h. The public create, the content check that every create shares, the journal replay and restore all use it, so the four checks cannot drift apart.No step, result or golden hash moves: the check only refuses snapshots the library cannot write. The height field and hull tails already run their create checks.
Verification
TestRestoreRefusesMeshUnderThreeVerticesintest/test_restore_links.cbuilds a mesh of 3 vertices whose one triangle uses only the first two. It cuts the mesh to 2 vertices, and to 1 vertex with the triangle rewritten to name vertex 0 at every corner, so both cuts pass the index check and only the vertex count is short. Each is refused with the world's hash unchanged and no misuse counted, and the clean snapshot still restores.-DMAUL3D_WERROR=ONpasses the test.tools/check.sh prpasses on this head, b2b7b11: 78 and 76 tests, clang-tidy clean, and fuzz with no finding.Closes #422