Fix 16-bit pointer truncation in sub-parser linked list#477
Merged
thedavidmeister merged 4 commits intomainfrom Apr 12, 2026
Merged
Fix 16-bit pointer truncation in sub-parser linked list#477thedavidmeister merged 4 commits intomainfrom
thedavidmeister merged 4 commits intomainfrom
Conversation
The sub-parser linked list packed a memory pointer into the top 16 bits of a bytes32 (shift 0xF0 = 240). When the free memory pointer exceeded 0xFFFF the pointer was silently truncated, corrupting the list. Widen to 96 bits (shift 0xA0 = 160) and extract the shift into a named constant SUB_PARSER_POINTER_SHIFT to prevent future inconsistencies. Closes #473 Closes #474 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request replaces hardcoded sub-parser pointer shifts with a named constant Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/src/lib/parse/LibParseState.pushSubParser.largeMemory.t.sol (1)
20-46: Solid regression test for the pointer truncation bug.The test correctly:
- Allocates 0x20000 bytes to push the free memory pointer past 0xFFFF
- Creates a minimal ParseState with required fields
- Pushes multiple sub-parsers to exercise the linked list
- Verifies round-trip correctness via
exportSubParsersConsider adding an assertion to verify the free memory pointer actually exceeded 0xFFFF, making the test more self-documenting and robust against future allocation changes.
Optional: Add assertion for memory pointer
// Allocate enough memory to push the free memory pointer past 0xFFFF. bytes memory padding = new bytes(0x20000); // Suppress unused variable warning. assembly ("memory-safe") { pop(padding) } + + // Verify the free memory pointer is past 0xFFFF (the bug trigger condition). + uint256 freeMemPtr; + assembly ("memory-safe") { + freeMemPtr := mload(0x40) + } + assertGt(freeMemPtr, 0xFFFF, "free memory pointer should exceed 0xFFFF");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/lib/parse/LibParseState.pushSubParser.largeMemory.t.sol` around lines 20 - 46, Add an assertion in testPushSubParserLargeMemoryOffset to explicitly verify the free memory pointer exceeded 0xFFFF after allocating padding: read mload(0x40) (the free memory pointer) via inline assembly after creating padding and assert it is > 0xFFFF; this should reference the existing padding allocation and test function name (testPushSubParserLargeMemoryOffset) so the purpose is clear and robust against future allocation changes.src/lib/parse/LibSubParse.sol (1)
3-3: Minor: Solidity version pragma should be exact=0.8.25.The guidelines specify exactly 0.8.25, but this file uses
^0.8.25.Proposed fix
-pragma solidity ^0.8.25; +pragma solidity =0.8.25;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/LibSubParse.sol` at line 3, Update the Solidity pragma in this file from a caret range to an exact version; specifically replace the `pragma solidity ^0.8.25;` declaration with the strict `pragma solidity =0.8.25;` so the compiler version is pinned exactly (locate the pragma at the top of LibSubParse.sol and update it accordingly).src/lib/parse/LibParseState.sol (1)
314-320: Outdated comment: pointer is now 96 bits, not 16 bits.The comment still states "the pointer for the linked list is ORed in the 16 high bits" but after this fix, the pointer occupies the high 96 bits.
Proposed fix
/// `@notice` Pushes a `uint256` representation of a sub parser onto the linked list of - /// sub parsers in memory. The sub parser is expected to be an `address` so - /// the pointer for the linked list is ORed in the 16 high bits of the - /// `uint256`. Only 16 bits are available for the linked-list pointer, so - /// this function relies on `checkParseMemoryOverflow` keeping the free - /// memory pointer below `0x10000`. If that invariant is violated, the - /// tail pointer will be silently truncated and the linked list corrupted. + /// sub parsers in memory. The sub parser is expected to be an `address` so + /// the pointer for the linked list is ORed in the high 96 bits of the + /// `uint256` (above the 160-bit address). This provides ample space for + /// any practical memory pointer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse/LibParseState.sol` around lines 314 - 320, Update the outdated comment in LibParseState.sol that describes how sub-parser pointers are encoded: change the text to state that the linked-list pointer is ORed into the high 96 bits (not 16 bits) of the uint256 and update the invariant description to reference that checkParseMemoryOverflow must keep the free memory pointer below 2**96 (0x1_0000000000000000000000000) so the high-96-bit pointer isn't truncated; ensure the comment mentions the functions/guards involved (the push routine in this file and checkParseMemoryOverflow) so readers know where the invariant is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/src/lib/parse/LibParseState.pushSubParser.largeMemory.t.sol`:
- Line 3: Update the Solidity version pragma at the top of the file by replacing
the loose pragma "pragma solidity ^0.8.18;" with the exact required version
specifier "pragma solidity =0.8.25;" so the contract/compiler uses the mandated
0.8.25 compiler; locate the existing pragma line (the "pragma solidity"
declaration) and change its version operator and number accordingly.
In `@test/src/lib/parse/LibParseState.pushSubParser.t.sol`:
- Line 3: The pragma in this test file currently uses a caret range ("pragma
solidity ^0.8.18;") which violates the guideline requiring an exact Solidity
version; update the Solidity version pragma to "pragma solidity =0.8.25;" in
this file (look for the top-level "pragma solidity" directive) and ensure any
nearby test helpers or imports in the same test suite are compatible with
0.8.25.
---
Nitpick comments:
In `@src/lib/parse/LibParseState.sol`:
- Around line 314-320: Update the outdated comment in LibParseState.sol that
describes how sub-parser pointers are encoded: change the text to state that the
linked-list pointer is ORed into the high 96 bits (not 16 bits) of the uint256
and update the invariant description to reference that checkParseMemoryOverflow
must keep the free memory pointer below 2**96 (0x1_0000000000000000000000000) so
the high-96-bit pointer isn't truncated; ensure the comment mentions the
functions/guards involved (the push routine in this file and
checkParseMemoryOverflow) so readers know where the invariant is enforced.
In `@src/lib/parse/LibSubParse.sol`:
- Line 3: Update the Solidity pragma in this file from a caret range to an exact
version; specifically replace the `pragma solidity ^0.8.25;` declaration with
the strict `pragma solidity =0.8.25;` so the compiler version is pinned exactly
(locate the pragma at the top of LibSubParse.sol and update it accordingly).
In `@test/src/lib/parse/LibParseState.pushSubParser.largeMemory.t.sol`:
- Around line 20-46: Add an assertion in testPushSubParserLargeMemoryOffset to
explicitly verify the free memory pointer exceeded 0xFFFF after allocating
padding: read mload(0x40) (the free memory pointer) via inline assembly after
creating padding and assert it is > 0xFFFF; this should reference the existing
padding allocation and test function name (testPushSubParserLargeMemoryOffset)
so the purpose is clear and robust against future allocation changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cb8ba7d-ddfd-44c8-be6f-8343c2ef91e3
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.locksrc/generated/Rainlang.pointers.solis excluded by!**/generated/**src/generated/RainlangExpressionDeployer.pointers.solis excluded by!**/generated/**src/generated/RainlangParser.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (6)
.gas-snapshotsrc/lib/parse/LibParseState.solsrc/lib/parse/LibSubParse.soltest/src/lib/parse/LibParsePragma.keyword.t.soltest/src/lib/parse/LibParseState.pushSubParser.largeMemory.t.soltest/src/lib/parse/LibParseState.pushSubParser.t.sol
test/src/lib/parse/LibParseState.pushSubParser.largeMemory.t.sol
Outdated
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/manual-sol-artifacts.yaml:
- Line 45: The workflow currently sets DEPLOYMENT_KEY to secrets.PRIVATE_KEY
unconditionally, allowing non-main manual runs to use the production signer;
change the DEPLOYMENT_KEY environment assignment so it selects a branch-scoped
secret or fails for non-main refs (e.g., set DEPLOYMENT_KEY to the production
secret only when github.ref or github.ref_name equals main, otherwise pick a
branch-specific secret or throw), ensuring script/Deploy.sol's
deployAndBroadcast path only sees the prod key for main runs and cannot
broadcast from other refs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62d391ef-340a-4c62-8f30-e4da605ec124
📒 Files selected for processing (1)
.github/workflows/manual-sol-artifacts.yaml
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
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
1 participant
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
Test plan
LibParseState.pushSubParser.largeMemory.t.soltest reproducing the truncation bug🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests
Chores