🐞 Fix LibBytes.split null path and dynamicStructInCalldata bounds check - #1562
Open
rakhmadaa-gif wants to merge 1 commit into
Open
Conversation
- indicesOf: when needle is longer than subject, allocate a proper
empty array instead of returning a null pointer. Previously, split
read the array length from the scratch space at 0x00, which is not
guaranteed to be zero, causing memory corruption or OOG on the
null path (e.g. split("", ",")).
- dynamicStructInCalldata: add the missing gt(s, l) bounds check,
mirroring bytesInCalldata. Previously a struct offset s > a.length
was not rejected and result.length = a.length - s wrapped around.
Co-Authored-By: Letta Code <noreply@letta.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two edge-case bugs in
LibBytes.sol, each with a deterministic failing test before the fix:1.
splitreads the array length from scratch space on the null pathWhen
needle.length > subject.length(e.g.split("", ",")),indicesOfskips its entire body and returns the default null pointer (0).splitthen computes:i.e. it loads the array length from address 0x00 (scratch space), whose content is not guaranteed to be zero between calls. With dirty scratch the null path corrupts memory (the FMP slot at 0x40 can get clobbered with
subject.lengthbefore the loop allocates elements) — deterministic repro: two consecutivesplitcalls with a longer delimiter OOG-revert on the second call.The clean-scratch path also synthesizes the returned
bytes[]header in scratch0x00–0x40rather than in allocated memory, so any later scratch use corrupts the returned array.Fix:
indicesOfnow allocates a proper empty array on theneedle.length > subject.lengthpath (including the one spare word so the array stays recyclable forsplit, per the existing allocation comment). With this,splitoperates on real allocated memory on every path.2.
dynamicStructInCalldatais missing thes <= a.lengthcheckThe bounds check only rejects
s >= 2^192andoffset > l, but nots > l:A malformed calldata blob with a struct offset word of 65 on a 64-byte input returns a slice with
length = 2^256 - 1instead of reverting. The siblingbytesInCalldatahas the equivalent check (gt(add(s, result.length), l)), so this looks like an omission rather than a design choice — the NatSpec says "Performs bounds checks".Fix: add
gt(s, l)to the revert condition, mirroringbytesInCalldata.Why the existing suite misses these
testStringSplitfuzzes withsubject = filler0 ++ delimiter ++ filler1, sodelimiter.length <= subject.lengthalways holds and the null path is never fuzzed. The static split tests do hit the null path (split("ab", " ")) but only with clean scratch in a fresh call frame.testDynamicStructInCalldata*only pass validabi.encode(...)outputs; the malformed-offset-word case is never exercised.Tests
test/LibBytesEdgeCases.t.soladds 5 tests (2 sanity, 3 that fail on current main and pass with the fix). Full suite: 1597/1597 pass with both fixes applied (forge test, 81 suites).Notes
indicesOfreturning a null pointer is itself unsound for direct callers too: readingresult.lengthon a null pointer loads from scratch. The fix addresses bothsplitand direct callers.