Uh oh!
There was an error while loading. Please reload this page.
test(LibConvert): pin aliasing, source lifetime and the write bounds - #19
Conversation
The two reference implementation tests compare only the value of the result, which leaves three behaviours unobserved. `unsafeToBytes` hands back the buffer it was given and rewrites the shared length prefix from a count of words to a count of bytes. That aliasing is the whole reason the source is unsafe to use afterwards, yet an implementation returning a copy passed the suite. Pin the pointer and the rewritten prefix. `unsafeTo16BitBytes` allocates its own result and leaves the source intact, which is the opposite of `unsafeToBytes` and worth stating. The packing loop writes whole words at two byte offsets, so it stops short of the end of what it allocated. A write past that end lands beyond the length the value comparison reads and so cannot be seen by comparing results. Place sentinels immediately above the allocation instead, and cover the lengths whose packed data exactly fills whole words, where one byte past the end of the data falls outside the allocation rather than into its padding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Warning Review limit reached
Next review available in:55 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The two reference implementation tests compare only the value of the result
against a slow reimplementation. That leaves three behaviours of
LibConvertentirely unobserved, each confirmed by a mutation that passed the whole suite.
unsafeToByteshands back the buffer it was given and rewrites theshared length prefix from a count of words to a count of bytes. That aliasing
is the entire reason the source is unsafe to use afterwards — the NatSpec says
so — yet an implementation that returns a copy instead passed the suite.
unsafeTo16BitBytesallocates its own result and leaves thesource intact, the opposite of
unsafeToBytes. An implementation that zeroedeach source word after reading it passed the suite.
it has to stop short of the end of what it allocated. A write past that end
lands beyond the length the value comparison reads, so comparing results cannot
see it. Both an end bound that doubled the span and an off-by-one loop
condition passed the suite while writing outside the allocation.
Sentinels placed immediately above the allocation catch the last of these. The
concrete case covers lengths whose packed data exactly fills whole words, where a
single byte past the end of the data falls outside the allocation rather than
into its padding.
QA
Discriminating tests:
testUnsafeToBytesAliasesTheSourceAndRewritesItsLengthPrefix,testUnsafeTo16BitBytesLeavesTheSourceIntact,testUnsafeTo16BitBytesWritesNothingPastItsAllocation,testUnsafeTo16BitBytesWritesNothingPastItsAllocationAtWordBoundaries- each fails on base under its mutation and passes unmutated, verified by re-runningmutation-probeover 24 mutants after the tests were added (23/24 killed, previously 19/23). Each compares exact values, never a bare revert: the returned pointer against the source pointer, the rewritten prefix againstwordLength * 32, every source element against an independent snapshot, and four sentinel words against the value written into them.Mutations applied: 24 against
src/LibConvert.sol, one behaviour each.bs := us->add(us, 0x20)(M14) ->testUnsafeToBytesAliasesTheSourceAndRewritesItsLengthPrefix;bs := us-> allocate and copy (M15, survived before) ->testUnsafeToBytesAliasesTheSourceAndRewritesItsLengthPrefix;mstore(bs, mul(0x20, mload(bs)))-> drop the scale /0x21/div(M16, M17, M18) -> same test plustestUnsafeToBytesReferenceImplementation;new bytes(us.length * 2)->* 1/* 3(M19, M20) ->testUnsafeTo16BitBytesWritesNothingPastItsAllocation;replaceMask0xFFFF->0xFF/0xFFFFFF(M21, M22) ->testUnsafeTo16BitBytesReferenceImplementation;preserveMask := not(replaceMask)->not(0)(M23) -> SURVIVES, equivalent mutant, see below; ->replaceMask(M24) -> reference test;cursor := add(us, 0x20)->us(M25) -> reference test;end := add(cursor, mul(mload(us), 0x20))->0x40(M26, survived before) ->testUnsafeTo16BitBytesWritesNothingPastItsAllocationand...AtWordBoundaries; ->sub(mload(us), 1)(M27) -> reference test;bytesCursor := add(bs, 0x02)->bs/add(bs, 0x04)(M28, M29) -> reference and canary tests;lt(cursor, end)->gt(M30) -> reference test; ->iszero(gt(cursor, end))(M31, survived before) ->testUnsafeTo16BitBytesWritesNothingPastItsAllocationand...AtWordBoundaries; strides0x20->0x40and0x02->0x04(M32, M33) -> reference and canary tests; masks swapped /mload(us)formload(cursor)(M34, M35) -> reference test;return bs->new bytes(0)(M36) -> reference test; zero each source word after reading it (M37, new mutant for the new behaviour) ->testUnsafeTo16BitBytesLeavesTheSourceIntact. Baseline green at both points: 6 tests before, 10 after.Oracle: the
LibConvertNatSpec, not the implementation.unsafeToBytesdocuments that "there is now two pointers to the same mutable data structure AND the length prefix for theuint256[]version is corrupt" — the pointer equality and the rewritten prefix assertions are read straight off that sentence.unsafeTo16BitBytesdocuments only truncation as its unsafety, and allocates its own result, so leaving the source intact is the behaviour its own NatSpec implies. The write-bounds assertions come from the meaning ofnew bytes(n)— an allocation of0x20 + roundUp32(n)bytes, a size independently measured against the compiler rather than assumed — not from reading the loop.Category check: no issue drives this PR; it is the coverage half of an adversarial mutation test run over the whole repo, so the categories are the mutation catalog's. Exercised against this unit: conditionals and comparisons (loop condition negated and off-by-one), arithmetic/off-by-one (length scale, allocation size, strides, end bound), returns/outputs (empty bytes), side effects (drop the length rewrite, add a write to the source), and constants/identifiers (mask literals, cursor bases, wrong source operand). Filters/scopes do not occur in this unit. Test-only change; no source file is touched.
M23 is reported SURVIVED and is left uncovered deliberately: it is an equivalent mutant, not a gap. Setting
preserveMaskto all ones turns the read-modify-write into an OR, which is identical to the shipped behaviour becausenew byteszero-initialises its allocation and each destination position is written exactly once, in increasing order. Verified rather than argued: a side-by-side fuzz comparison of the shipped implementation against thenot(0)variant over 1024 runs found both bit-identical across the whole padded allocation, not merely the logical value. Writing a test that "kills" it would mean pinning an implementation detail the shipped code deliberately does not depend on.🤖 Generated with Claude Code