Uh oh!
There was an error while loading. Please reload this page.
Check what the caller supplied Vm returned before stripping the 0x prefix - #52
Conversation
…efix bytesToHex takes its Vm as a parameter, so the string it strips two characters from is whatever the caller's Vm returns. A shorter return underflows the length word and an unprefixed one silently discards two characters of real data. Require "0x" followed by two hexadecimal characters per input byte, which is what foundry's own Vm returns. Closes#43 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mutation pass survived dropping the check on the first prefix character: every case with a wrong first character also had a wrong second one, so the second check alone rejected them all. Add the mirror of the wrong-second-character case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesHex string validation
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
thedavidmeister
commented
Aug 16, 2026
@coderabbitai review |
|
The check read the length twice — once as `hexBytes.length` and again as `mload(hexString)` in the block below it — and reached the prefix through two bounds-checked index reads, while the block directly underneath re-derived the pointer it had just walked away from. The check and the code whose correctness depends on it were two constructs a later edit can drift apart. One block now: one length load feeding both the comparison and the strip, the prefix as the top two bytes of the data word against `0x3078`, and the strip running only on the valid path rather than after a separate guard. The length is checked in an outer `if` and the prefix read inside it, because `and` in Yul evaluates both arms — a single `and` would read the data word even for a zero-length string, past that string's own allocation, which is the claim `memory-safe` makes. `expectedLength` is at least 2, so a length that matches it puts the word inside the allocation. The revert stays in solidity. `UnexpectedHexString` carries a dynamic string, and hand-encoding one in assembly is a dozen lines of pointer arithmetic in a build-time function where gas is not a consideration. 95 tests pass, 31 of them LibHexString's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister
commented
Aug 16, 2026
Reviewed e01ae9e: ready — Closes #43.
The decision to fix rather than close is argued on the record and holds: the trusted value is a function parameter, not the cheatcode address, so it is untrusted by construction; #37 already asserts exactly this property over 2048 runs, but only where it protects this repo's CI rather than a consumer's build; and the function is The final commit reshapes the guard into the existing assembly block on the user's call. One length load feeds both the comparison and the strip, the prefix is the top two bytes of the data word against The length is checked in an outer 95 tests pass, 31 of them LibHexString's, and Two limits on the evidence, stated rather than implied. The 36-mutant matrix in the body was run against |
Uh oh!
There was an error while loading. Please reload this page.
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bashset -euo pipefail
printf'%s\n''== PR metadata =='
gh pr view 52 --repo rainlanguage/rain.sol.codegen --json baseRefOid,headRefOid,changedFiles,title,url
printf'%s\n''== Changed files =='
git diff --name-status "$(git merge-base HEAD origin/HEAD)" HEAD 2>/dev/null || git diff --name-status HEAD~1 HEAD
printf'%s\n''== LibHexString outline =='
ast-grep outline src/lib/LibHexString.sol --items all --view expanded
printf'%s\n''== Relevant source locations =='
rg -n -C 12 'bytesToHex|UnexpectedHexString|memory-safe' src/lib/LibHexString.sol
printf'%s\n''== Relevant test locations =='
fd -a 'LibHexString.*bytesToHex.*\.sol|NonConformingVm\.sol'test| sort |whileread -r f;doecho"--- $f"
rg -n -C 3 'bytesToHex|UnexpectedHexString|NonConformingVm'"$f"||truedoneprintf'%s\n''== Diff against PR base =='
base="$(gh pr view 52 --repo rainlanguage/rain.sol.codegen --json baseRefOid --jq .baseRefOid)"
git diff --find-renames --find-copies --unified=80 "$base"...HEAD -- src/lib/LibHexString.sol testLength of output: 41037
I found no blocking issues. The nested conditions prevent a prefix-word read until the length check proves that the allocation contains at least two bytes. The valid path changes the pointer and length only after both checks pass. The invalid path preserves the original string for The tests cover the relevant malformed return categories and the empty-data boundary. The fuzz test also matches the guard predicate. ✏️ Learnings added
You are interacting with an AI system. |
thedavidmeister
commented
Aug 16, 2026
@coderabbitai review |
|
Closes#43.
The decision: check it
The issue frames the real tension —
Vmis foundry's, and a library re-checkingits own cheatcode's output is unusual — and leaves the call open. This PR adds
the check. The reasoning, because the counter-argument is a good one:
The thing being trusted is a parameter, not a cheatcode.
bytesToHex(Vm vm, bytes memory data)does not call the cheatcode address; it calls whateveraddress the caller cast to
Vm.Vm(anyAddress)type checks. So the string thepointer arithmetic operates on is, by the shape of the function signature, an
untrusted input — the same category as
data. "Do not re-check foundry" would bethe right answer if the library reached for
VM_ADDRESSitself, and the honestalternative fix is exactly that (see below); it is not the right answer for a
value that arrives through the parameter list.
The failure is silent, and what it corrupts is source that compiles. The
""and"Z"rows of the issue's table produce a length word near2**256,which is loud in the sense that anything downstream will run out of gas. The
"abcdef"row is the one that matters: two characters of real data are droppedand the result is spliced into
hex"..."in generated Solidity, giving abytes constantthat compiles, commits, and holds the wrong value. For arepository whose entire product is generated source, a wrong constant that
compiles is the worst available failure class, and a revert is strictly better.
There is no counterweight.
bytesToHexisinternalandpure, reachedonly from build scripts and tests, and it already makes an external call. Two
comparisons cost nothing that anyone is measuring.
The property is already asserted — in the wrong place.#37 landed
testBytesToHexIsVmToStringWithoutPrefix, which asserts over 2048 fuzz runs thatvm.toString(data)is2 * data.length + 2characters beginning0x. The repoalready agrees this property is load bearing. But that assertion only runs in
this repo's CI; a downstream consumer runs the generator, not this suite. Moving
the same predicate into the function is what makes it hold at every consumer's
build. The check here is deliberately the same predicate, not a weaker one.
What changed
src/lib/LibHexString.sol:bytes(hexString)is a free cast on the same pointer, so nothing is copied. Thelength equality short circuits ahead of the two index reads, so those are always
in bounds. The docstring now states the revert; the error is a typed
UnexpectedHexString(string hexString, uint256 expectedLength)carrying theoffending string rather than a message to match on.
Also the
///that #43 notes at the end — the continuation line of the in bodycomment inside the assembly block used the NatSpec marker where
//is meant.Why the length is an equality and not
>= 2>= 2is the minimal fix for the underflow and it is not enough: it leaves the"abcdef"row, which is the row the issue calls the one that matters, and itleaves a
Vmreturning"0xaa"forhex"aabb"emitting a constant holding halfthe data. The property the function actually depends on is "the return is
toString(bytes)'s defined output", and its structural half — length and prefix —is checkable in two comparisons. The remaining half, that the nibbles are the
right nibbles, is not checkable without reimplementing the encoder, so it is out.
That is where the line falls, and it is drawn at a property rather than at the
list of inputs the issue happened to enumerate.
The alternative that was rejected
The other way to close this is to stop taking
Vmas a parameter and useVm(VM_ADDRESS)internally, which removes the untrusted input rather thanchecking it. That is a strictly better answer to the narrow question and a much
worse change:
Vmis threaded as a parameter through all 11 public functions ofLibCodeGenand both ofLibFs, so it is an API break for every consumer, andit forecloses the
vm.prank-style setups a consumer might legitimately want. Notin this PR's scope, and not obviously wanted at all.
The check is on the shape of the returned string, not on the identity of the
Vm, so it does not narrow the parameter by the back door — a non-foundryVmthat conforms still works, which
testBytesToHexAcceptsConformingVmpins.Tests
11 new tests in
test/lib/LibHexString.bytesToHex.t.sol, andtest/concrete/NonConformingVm.sol— atoString(bytes)-shaped contractreturning a string fixed at construction. It is its own file because the repro
attached to the issue declared two contracts in one, which
rainix staticrejects. The library call is forced through the existing
LibHexStringExternalwrapper, because
bytesToHexisinternalandexpectRevertonly sees a revertthat happens in a subcall.
Each of the issue's three rows gets a test, plus the cases the check implies:
truncated but correctly prefixed, overlong, a wrong first prefix character with a
correct second, a wrong second with a correct first, the empty-data boundary
where the required length is 2 rather than 0, both accepting cases, and a fuzz
test over
(bytes data, string toStringReturn)asserting the whole property —for any data and any string a
Vmmight return, the library either reverts orreturns exactly that string with its first two characters removed.
The wrong-first-character test exists because the mutation pass found it
missing: dropping the check on
hexBytes[0]initially SURVIVED, since everynon-conforming string in the suite had a wrong second character too and the
second check alone rejected all of them. Second commit on this branch, with the
mutant re-run to confirm it now dies.
No existing test was weakened, edited or deleted. The suite goes 84 -> 95.
Pre-existing red, unrelated, checked not assumed
forge test --via-irfailstestBuildFileForContractCommittedArtifactIsCurrent,because the committed
src/generated/CodeGennable.solembeds aBYTECODE_HASHproduced by the pipeline
foundry.tomlconfigures. Verified this failsidentically on the base commit
af4e5a9with these changes reverted (83 passed /1 failed there, 93 / 1 here — the same single test). Not introduced here and not
fixed here; the default pipeline, which is what CI runs, is 94/94 green.
QA
Discriminating tests: 10 new tests, every one asserting an exact revert payload
(
abi.encodeWithSelectorwith a literal expected length, not the source's ownformula) or an exact returned string, never "it reverted". Full suite 95 tests
/ 15 suites, 0 failures, from an 84/0 baseline on
af4e5a9;forge fmt --checkclean; CI green on this branch,
rainix / staticandrainix / testbothsuccess on the head commit
c3aaa42athttps://github.com/rainlanguage/rain.sol.codegen/actions/runs/31958846546
(
staticis what enforces one contract per file, hence the separateNonConformingVm.sol). The 31LibHexStringtests were also run under thepipelines LibHexString.bytesToHex gets a test file #37 established as the bar —
--via-ir,--evm-version shanghaiwhere there is no
mcopy, and--via-ir --evm-version paris— 31/31 undereach, because the guard introduces a second live reference to the same buffer
the memory-safe block mutates.
Mutations applied: 36, run with
mutation-probeagainst the whole suite from agreen baseline — 18 on the new check, and all 18 of LibHexString.bytesToHex gets a test file #37's mutants on the
adjacent prefix strip re-run, because a guard inserted immediately in front of
that assembly could have masked a mutant that used to die. 34/36 killed on the
first pass; both survivors are named below and one of them was a real gap that
this branch then closed. Killer lists are the first the probe reports, not the
complete set.
2 * len + 2->2 * len + 1testLiteralParserFunctionPointersConstantString,testOperandHandlerFunctionPointersConstantString,testOpcodeFunctionPointersConstantString2 * len + 2->2 * len + 32 * len + 2->2 * len(prefix unaccounted)testSubParserWordParsersConstantString,testOpcodeFunctionPointersConstantString,testBytesConstantString2 * len + 2->len + 2(one char per byte)testLiteralParserFunctionPointersConstantString,testOperandHandlerFunctionPointersConstantString,testOpcodeFunctionPointersConstantString2 * len + 2->3 * len + 2testBytesConstantString,testBytesConstantStringAtMaxLength,testBytesConstantStringCarriesDatatestBytesToHexRejectsEveryNonConformingVmOutput,testBytesToHexRevertsOnEmptyVmOutput,testBytesToHexRevertsOnOneCharacterVmOutputfalsetruetestLiteralParserFunctionPointersConstantString,testIntegrityFunctionPointersConstantString,testOperandHandlerFunctionPointersConstantStringtestBytesToHexRevertsOnEmptyVmOutput,testBytesToHexRevertsOnOverlongVmOutput,testBytesToHexRevertsOnTruncatedVmOutputlength != expected->length < expectedtestBytesToHexRejectsEveryNonConformingVmOutput,testBytesToHexRevertsOnOverlongVmOutputlength != expected->length == expectedtestOperandHandlerFunctionPointersConstantString,testSubParserWordParsersConstantString,testLiteralParserFunctionPointersConstantStringtestBytesToHexRevertsOnWrongFirstPrefixCharactertestBytesToHexRevertsOnWrongSecondPrefixCharacterbytes1("0")->bytes1("1")testSubParserWordParsersConstantString,testLiteralParserFunctionPointersConstantString,testOpcodeFunctionPointersConstantStringbytes1("x")->bytes1("X")testIntegrityFunctionPointersConstantString,testOpcodeFunctionPointersConstantString,testSubParserWordParsersConstantStringtestOpcodeFunctionPointersConstantString,testOperandHandlerFunctionPointersConstantString,testBytesConstantStringtestBytesToHexRevertsOnOneCharacterVmOutput,testBytesToHexRevertsOnTruncatedVmOutput,testBytesToHexRevertsOnUnprefixedVmOutputtestBytesToHexRevertsOnEmptyVmOutput,testBytesToHexRevertsOnEmptyVmOutputForEmptyData,testBytesToHexRevertsOnOneCharacterVmOutputtestBytesToHexAcceptsConformingVm,testBytesToHexAllocationBoundary,testBytesToHexCharsettestBytesToHexAcceptsConformingVm,testBytesToHexAcceptsConformingVmForEmptyData,testBytesToHexCharsettestBytesToHexAcceptsConformingVm,testBytesToHexIsVmToStringWithoutPrefix,testBytesToHexKnowntoString(bytes("")))testIntegrityFunctionPointersConstantString,testSubParserWordParsersConstantString,testLiteralParserFunctionPointersConstantStringreturn hexString->return ""testBytesToHexAcceptsConformingVm,testBytesToHexConcatenatesIntoHexLiteral,testBytesToHexIsVmToStringWithoutPrefixmstore(0x80, 0)testBytesToHexDoesNotMutateInput,testBytesToHexLeavesNeighbouringMemoryAlone,testBytesConstantStringAtMaxLengthassembly ("memory-safe")->assemblytestBytesToHexAcceptsConformingVm,testBytesToHexAllocationBoundary,testBytesToHexCharsettestBytesToHexDoesNotMutateInput,testBytesToHexLongData,testBytesToHexRoundTripsN12 was a genuine hole: every non-conforming string in the first version of the
suite had a wrong SECOND prefix character as well as a wrong first, so the
second check alone rejected all of them and the first was never exercised
alone. Fixed by
testBytesToHexRevertsOnWrongFirstPrefixCharacter(
"Zxaabb"), and N12 and N13 re-run afterwards: 2/2 killed, N12 by exactlythat test.
M16 is the same non-behaviour control LibHexString.bytesToHex gets a test file #37 used and its survival is still the
correct verdict, not a gap: the annotation is a promise to the optimiser and
removing it only makes the optimiser more conservative. What the annotation
claims is probed directly by M15, M17 and M18, all killed.
No mutant of the new guard survived on the prefix strip, so the guard does not
mask any of LibHexString.bytesToHex gets a test file #37's coverage — all 18 M-mutants still die, several of them now
additionally through the new conforming-
Vmtests.Oracle: intent came from the definition of
toString(bytes)and from what solcaccepts, not from what the library returns. The fuzz test derives conformance
independently (
length == 2n + 2 && [0] == '0' && [1] == 'x') and asserts thestripped string it computes itself, so it cannot agree with a wrong
implementation. The accepting cases are asserted through a non-foundry
Vmaswell as the real one, which is what proves the guard gates on the string's
shape and not on the caller's
Vmbeing foundry's. Note the N01-N05, N08,N11, N14-N16 killers are all
LibCodeGentests: mutating the required shapemakes the real
vmfail the check, which is the independent confirmation thatwhat the guard demands is exactly what foundry returns.
Category check: the issue lists three offending returns; the category is every
way a return can fail the two facts the strip depends on. Enumerated as: too
short to index (0 and 1 characters), right length with no prefix, right length
with a wrong first prefix character only, right length with a wrong second only,
correctly prefixed but too short, too long, and the
data.length == 0boundarywhere the required length is 2. All seven have a test, and the fuzz test covers
the category as a property rather than as the list.
Summary by CodeRabbit
Bug Fixes
Tests