fix[venom]: port augassign read/write overlap guard#5099
Open
banteg wants to merge 2 commits into
Open
Conversation
legacy codegen rejects augmented assignments whose RHS can mutate the assignment target (GHSA-4w26-8p97-f4jp), since the target pointer is computed and bounds-checked before the RHS is evaluated. the venom pipeline was missing this guard, so e.g. `self.a[1] += self.a.pop()` would compile and write out-of-bounds at runtime. port the guard from vyper/codegen/stmt.py:parse_AugAssign to lower_AugAssign using AST-level equivalents of the legacy IRnode properties (referenced_variables, variable_writes, contains_writeable_call), so the same source programs are rejected with the same CodegenPanic.
Member
|
i think for venom we should actually just do the right thing rather than rejecting the program. i'd be ok with this as an interim fix tho |
Contributor
Author
|
Looked at this alongside #5100. I left this PR as the interim augassign guard for now: unlike rvalue subscript lowering, augassign writes back after RHS evaluation, so making it non-rejecting needs a broader write-semantics decision rather than the stale-base-pointer fix. #5100 now implements the narrower stale-base handling requested there. |
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 #5051.
The Venom pipeline's
lower_AugAssignwas missing the read/write overlap guard from GHSA-4w26-8p97-f4jp: the target pointer is computed (and bounds-checked) before the RHS executes, so programs likeself.a[1] += self.a.pop()compiled under--experimental-codegenand could write out of bounds after the array shrank. Legacy rejects this class at codegen time (parse_AugAssign, added in #4487, relaxed in #4497).How I did it
Legacy's check reads legacy-IRnode properties (
referenced_variables,variable_writes,contains_writeable_call), which don't exist in the AST-based venom lowering — but each has a faithful AST-level equivalent seeded from the same frontend analysis legacy itself seeds from:referenced_variables→ recursive collection of_expr_info._readsover the reduced target AST (the frontend folds internal callees' state reads into call nodes);variable_writes→get_expr_writes(rhs)(the exact function legacy uses to seed IRnodes; internal-call writes are tracked transitively by the frontend);contains_writeable_call→extcallnodes, non-staticraw_call(viaget_mutability_at_call_site, sois_static_call=Truestays allowed like legacy),send, and create builtins, recursing transitively through internal callees viareachable_internal_functions.Same prim-word-target relaxation, same
CodegenPanic("unreachable").How to verify it
The GHSA source lists in
test_assignment.pyare extracted into module constants and exercised underSettings(experimental_codegen=True): 6 rejection cases (all fail on master — they compile when they shouldn't) plus legacy-accepted overlap-OK sources still compiling. A 12-case differential confirmed venom now matches legacy exactly (6 rejected / 6 accepted), including a state-writing extcall hidden behind a transitive internal call.tests/unit/compiler/plus a 2094-test venom functional sweep pass with no over-rejection.Commit message
Description for the changelog
Fix: experimental codegen rejects augmented assignments whose RHS can mutate the assignment target (GHSA-4w26-8p97-f4jp parity).
Cute Animal Picture