Hardening: dense meet operand fix, a stacked-borrows UB fix, and two guards#45
Hardening: dense meet operand fix, a stacked-borrows UB fix, and two guards#45MesTTo wants to merge 5 commits into
Conversation
|
The changes to LineListNode seem to hint at a bug-fix, can you give an explanation of the bug? The test for it is quite impenetrable to me. |
|
The LineListNode change is not fixing a newly observed runtime failure by itself... It is guarding an existing layout invariant... LineListNode is written around a fixed in-memory layout. The key byte capacity is chosen so the node stays at 64 bytes with |
| @@ -1850,7 +1854,12 @@ impl<V: Clone + Send + Sync, A: Allocator, Cf: CoFree<V=V, A=A>, OtherCf: CoFree | |||
| if new_mask > 0 { | |||
| AlgebraicResult::Identity(new_mask) | |||
| } else { | |||
| AlgebraicResult::Element(Self::new(None, self.val().cloned())) | |||
| let val = if val_mask & SELF_IDENT > 0 { | |||
| self.val().cloned() | |||
| } else { | |||
| other.val().cloned() | |||
| }; | |||
| AlgebraicResult::Element(Self::new(None, val)) | |||
| } | |||
| }, | |||
| (AlgebraicResult::Identity(rec_mask), AlgebraicResult::None) => { | |||
| @@ -1864,7 +1873,12 @@ impl<V: Clone + Send + Sync, A: Allocator, Cf: CoFree<V=V, A=A>, OtherCf: CoFree | |||
| if new_mask > 0 { | |||
| AlgebraicResult::Identity(new_mask) | |||
| } else { | |||
| AlgebraicResult::Element(Self::new(self.rec().cloned(), None)) | |||
| let rec = if rec_mask & SELF_IDENT > 0 { | |||
| self.rec().cloned() | |||
| } else { | |||
| other.rec().cloned() | |||
| }; | |||
| AlgebraicResult::Element(Self::new(rec, None)) | |||
There was a problem hiding this comment.
Sorry I meant DenseNode -- i.e. the motivation behind these.
|
ah right, denseNode not linelist... that's the actual fix, sorry. it's in that's only right when the surviving half is identity-to-self. in a commutative op the half can be identity-to-other instead (SELF_IDENT cleared, COUNTER_IDENT set), and then cloning self's half drops the wrong operand's value/child into the node. so the fix reads the bit: the giveaway is the the line 698 hunk in no change for the self-identity case, it only diverges when the surviving half matched other. |
combine_algebraic_results copied the left operand unconditionally in the two mixed value-only / rec-only branches; node_get_payloads treated an exact value-only request as exhaustive even when the CoFree held an unrequested onward link. Both could assemble a result from the wrong child or let a left-only chain survive an intersection. Select the operand by the SELF_IDENT mask, and mark the value-only enumeration non-exhaustive when an onward link is present. A 512-seed dual-distributivity differential vs a BTreeSet oracle (+ the seed-44 meet-associativity regression) failed 83 seeds before, 0 after. MORK's atom_intersection and DNF meet hit this on prefix-nested unit-keyed data. (cherry picked from commit 2619d9fba5b51e77c5a37d715f3b9faedac834ba)
with_k built K raw pointers with xs.as_mut_ptr().add(idx) inside a loop; each as_mut_ptr() re-borrowed the slice and invalidated the pointers from earlier iterations, so the later &mut *p retag hit a tag no longer in the borrow stack (Miri UB, test_almost_identical_paths_n). Replace with slice::get_disjoint_mut over the distinct bitmask indices: safe, no unsafe, same result, negligible cost for small K. Removes an unsafe block; Miri-clean. Verified: cargo +nightly miri test on this exact tree aborts at the claimed line (with_k's ptrs.map(|p| &mut *p)) before this fix, and is clean (120/120 zipper_algebra tests) after it.
deserialize_file indexed into the deserialized-records vec by offsets read
straight from the serialized file, so a malformed forward or out-of-range
offset panicked (index out of bounds) instead of returning an error. Bound
every offset lookup with .get().ok_or_else(...) so malformed input is
rejected, not a panic.
Verified: on this exact tree, a crafted forward-offset fixture
("P x...1x...2" with an empty records vec) panics at serialization.rs:820
without this fix; with it, deserialize_file returns Err("...offset out of
bounds") and both new regression tests pass.
Pins LineListNode<[u8;1024]> to 64 bytes under slim_ptrs on x86_64 (excluding miri, which pads for its own bookkeeping) with a compile-time size assert, so a layout change that grows the node is caught at build time instead of surfacing as a memory regression later. Only the slim_ptrs arm is asserted, for a concrete reason: the not(slim_ptrs) TrieNodeODRc has no empty-sentinel representation, so the shared call sites' new_empty/is_empty/make_unique/== do not exist there and that configuration does not compile (26 errors, all one root cause). Giving the Arc-backed variant a sentinel is the open design flagged in the TaggedNodeRefMut EmptyNode note (the allocator prevents a free empty sentinel); once that lands, the second size assert follows.
The test passes on this tree (verified via -- --ignored before removing the attribute), so the ignore is stale: the round-trip it covers works, and keeping it ignored just forfeits the regression coverage. If tree serialization still has known gaps elsewhere, this pins the part that does work.
MesTTo
force-pushed
the
pr/pathmap-hardening
branch
from
a3c01de to
064b320
Compare
MesTTo
changed the title
2 participants
Summary
Five small, independent correctness commits, each proven against a stock
masterreproduction before the fix:differential added in this commit fails 2/4 on stock
master(
combine_algebraic_resultswas copyingselfunconditionally in a casewhere the counter side held the correct operand); with the fix it's 4/4.
zipper_merge_n_monovia safeget_disjoint_mut— miri (stacked borrows) aborts on stockmasteratthe raw-pointer loop in
with_k; replacing it with the stable (≥1.86)get_disjoint_mutis miri-clean.masterpanics ondeliberately corrupted offset bytes during deserialization; this converts
the failure into a proper
Err.LineListNode<[u8;1024]>at 64 bytes underslim_ptrson x86_64. Only thatconfiguration is asserted: building
not(slim_ptrs)currently fails (26errors, one root cause —
TrieNodeODRc's Arc-backed variant has noempty-sentinel representation, so
new_empty/is_empty/make_unique/==don't exist there). That's the open question the
TaggedNodeRefMut::EmptyNodeGOAT note already flags; happy to take a pass at it once there's a
preferred sentinel design.
tree_serde_2test — verified passing on this tree; the#[ignore]was stale.Test plan
cargo test --release(663/0/0, up from the 660/0/1 baseline)cargo test --release --test pathmap_algebra_differential(4/4)cargo miri test --releaseover the touched zipper_algebra tests (120/120)cargo test --release --doc(7/0)