fix[venom]: touch immutables region before constructor body#5098
Open
banteg wants to merge 1 commit into
Open
Conversation
the venom constructor path reserves the immutables staging allocation at memory position 0 but never touched it at runtime, so msize-based dynamic allocation (`memtop`, used by raw_call(msg.data), create_copy_of and create_from_blueprint) could return a pointer inside the uninitialized staging region and clobber it. emit the equivalent of the legacy `iload(immutables_len - 32)` guard (vyperlangGH-3101): an `istore` of zero to the last word of the staging region, before constructor args are decoded. `istore` is volatile and carries the IMMUTABLES effect, so no optimizer pass can remove it, and a zero write to virgin memory is semantically a no-op. fixes vyperlangGH-5053
Member
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.
What I did
Fixes #5053.
The Venom constructor path reserved the immutables staging buffer at memory position 0 but never touched it at runtime, so msize-based dynamic allocation (
memtop— used byraw_call(msg.data),create_copy_of,create_from_blueprint) could return a pointer inside the uninitialized staging region. Legacy guards against this since GH-3101 by emitting["iload", max(0, immutables_len - 32)]before the init code.How I did it
In
_generate_constructor, inside the existingimmutables_len > 0block (after the position-0 alloca, before constructor-arg decoding and body lowering), emitistore(immutables_alloca + max(0, immutables_len - 32), 0). In the constructor contextistorelowers to a plainMSTOREat the staging address; storing zero is a no-op (first memory write of deploy execution), but it bumps MSIZE past the whole region.istorewas chosen deliberately so the guard survives optimization: it is inVOLATILE_INSTRUCTIONS(never removed byRemoveUnusedVariables), carries the IMMUTABLES effect (memory DSE skips it viahas_other_effects), and no load/store-elision pass special-cases it. Verified the guard reaches final IR and assembly at-O gas,-O codesize, and-O none.Notable: the existing GH-3101 functional tests pass under venom even without this fix, because their constructors take arguments — arg decoding already bumps msize past the staging region. The real trigger is an argless constructor doing dynamic allocation first; the new functional test deploys via blueprint and does
create_copy_of(msg.sender)before reading an immutable, which observes copied-code garbage on master.How to verify it
tests/unit/compiler/venom/test_ctor_msize_guard.pyasserts the guard in optimized deploy IR and asm (and its absence without immutables);test_immutables_initialized3is the runtime repro, passing under both pipelines. Both fail on master under--experimental-codegen. Fulltests/unit/compiler/plus immutables/create/raw_call functional suites pass under both pipelines.Commit message
Description for the changelog
Fix: experimental codegen touches the immutables staging region before the constructor body, so msize-based dynamic allocation cannot overlap it (GH-3101 parity).
Cute Animal Picture