fix[codegen]: fix removal of side effects in concat #4644
Conversation
concat would remove side effects for zero-length arguments. fix by removing the fastpath. as the test case shows, this pattern is not common in user-code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4644 +/- ##
=======================================
Coverage 92.53% 92.53%
=======================================
Files 129 129
Lines 18604 18605 +1
Branches 3228 3227 -1
=======================================
+ Hits 17215 17216 +1
Misses 943 943
Partials 446 446 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| @external | ||
| def test() -> Bytes[256]: | ||
| a: Bytes[256] = concat(b"" if self.sideeffect() else b"", b"aaaa") |
There was a problem hiding this comment.
just for completness noting that in this case we will hit this fast-path in copy_bytes
Line 373 in 9486a41
|
this fix should be sufficient as now we will hit the following line for all the arguments: vyper/vyper/builtins/functions.py Line 569 in 86b2868 and thus if the argument will be complex (and potentially have side-effects) it will get evaluated |
|
charles-cooper#85 shows a poc for side-effect elimination in case the |
…t-zero-length add concat side-effect elimination test
| @@ -557,10 +557,6 @@ def build_IR(self, expr, context): | |||
| dst_data = add_ofst(bytes_data_ptr(dst), ofst) | |||
|
|
|||
| if isinstance(arg.typ, _BytestringT): | |||
| # Ignore empty strings | |||
| if arg.typ.maxlen == 0: | |||
There was a problem hiding this comment.
i think this will have to be rewritten to smth like if arg.is_empty_intrinsic because of: https://github.com/vyperlang/vyper/pull/4649/files#diff-452732981c7a1a928e9f249c7b14c723695de3d6635568ccc8899b6cab2fc54bR151
.. otherwise bytes_data_ptr(arg) would fail i think
This reverts commit 6b88100. it's in PR vyperlang#4644
per title. as in the referenced pull requests and security advisories, it is difficult, but not impossible, to construct an empty bytestring which has side effects. in this commit, don't fastpath out the side effects for zero-length bytestrings references: - #4644 - GHSA-qhr6-mgqr-mchm - #4645 - GHSA-3vcg-j39x-cwfm --------- Co-authored-by: cyberthirst <[email protected]>
vyper/codegen/ir_node.py
Outdated
| @@ -369,6 +373,8 @@ def is_empty_intrinsic(self): | |||
| return True | |||
| if self.value == "seq": | |||
| return len(self.args) == 1 and self.args[0].is_empty_intrinsic | |||
| if self.is_source_literal and isinstance(self.typ, _BytestringT) and self.typ.maxlen == 0: | |||
There was a problem hiding this comment.
please add a comment why this counts as the empty intrinsic
vyper/codegen/expr.py
Outdated
| ["seq"] + seq + [placeholder], | ||
| typ=btype, | ||
| location=MEMORY, | ||
| annotation=f"Create {btype}: {bytez}", | ||
| ) | ||
| ret.is_source_literal = True |
There was a problem hiding this comment.
i would probably rename this to is_source_bytes_literal because other literals don't have this flag turned on. so that we don't use this in some future analysis with the wrong assumptions
concat would remove side effects for zero-length arguments. fix by removing the fastpath.
as the test case shows, this pattern is not common in user-code.
fix for GHSA-qhr6-mgqr-mchm
What I did
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture