Fix array assignment and deletion at storage slot overflow boundary #15984
Conversation
r0qs
force-pushed
the
storageOverflowDeleteBug
branch
2 times, most recently
from
cbd97cc to
b14ee0a
Compare
r0qs
force-pushed
the
storageOverflowDeleteBug
branch
10 times, most recently
from
550b207 to
1614491
Compare
r0qs
force-pushed
the
storageOverflowDeleteBug
branch
from
1614491 to
0d84691
Compare
| function f() public returns (uint256[10] memory) { | ||
| uint256[10][1] storage x = getArray(); | ||
| for (uint i = 0; i < 10; i++) | ||
| x[0][i] = i; | ||
| delete x[0]; | ||
| return x[0]; | ||
| } | ||
| } | ||
| // ---- | ||
| // f() -> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 |
There was a problem hiding this comment.
You should split this into two operations (fill and clear) and check the state in between. The behavior at the boundary is suspect in general, not just for delete. If the filling part is broken too, we won't notice otherwise.
The same applies to the other tests in this PR. Especially in delete_overflow_bug_large_mapping_storage_boundary.sol, you are not checking the values between the partial assignment and delete so if the PR fixed delete, but not the assignment, we would not notice.
There was a problem hiding this comment.
Also, you should use a getter for reading the values in expectations instead of returning it from the function. Otherwise actual storage reads and writes are getting optimized out by LoadResolver. It may not affect the result in practice, since the optimizer should preserve the semantics of generated Yul no matter if it's correct or not, but if we can eliminate this confounding variable, we should.
There was a problem hiding this comment.
You should split this into two operations (
fillandclear) and check the state in between. The behavior at the boundary is suspect in general, not just fordelete. If the filling part is broken too, we won't notice otherwise.The same applies to the other tests in this PR. Especially in
delete_overflow_bug_large_mapping_storage_boundary.sol, you are not checking the values between the partial assignment anddeleteso if the PR fixeddelete, but not the assignment, we would not notice.
Yeah, indeed, thanks for pointing that out. I actually noticed that assignments still seem to be broken with the current fix. The problem seems to be in the copyArrayToStorage function.
test/libsolidity/semanticTests/storage/delete_overflow_bug_collision.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/storage/delete_overflow_bug_large_mapping_storage_boundary.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/storage/delete_overflow_bug_large_mapping_storage_boundary.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/storage/delete_overflow_bug_partial_assignment.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/semanticTests/storage/delete_overflow_bug_partial_assignment.sol
Outdated
Show resolved
Hide resolved
|
Please include a link to the issue you're fixing in the PR description. |
r0qs
changed the title
r0qs
force-pushed
the
storageOverflowDeleteBug
branch
2 times, most recently
from
25bd6b5 to
b57d7a2
Compare
r0qs
force-pushed
the
storageOverflowDeleteBug
branch
from
b57d7a2 to
47471c6
Compare
Fixes #15587