Fix ICE when assigning calldata Struct with external function type member to storage variable. - #16764
Conversation
03cb6af to
55d0c56
Compare
55d0c56 to
0a3f7a4
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
| * General: Remove support for the experimental EOF (EVM Object Format) backend. | ||
|
|
||
| Bugfixes: | ||
| * Codegen: Fix ICE when assigning a calldata struct containing a member of external function type to a storage struct. |
There was a problem hiding this comment.
Gonna have to move this up into 0.8.37.
There was a problem hiding this comment.
I would add a few more tests:
- struct with external function ptr + uint64 (packing)
- struct with external function ptr + uint256[] array (dynamic)
- array of structs with external function ptr, i.e.
contract C {
S[] storageStruct;
....
}
62d0bde to
b6deae8
Compare
b6deae8 to
7fba57e
Compare
nikola-matic
left a comment
There was a problem hiding this comment.
I've got another test with malformed function pointers:
struct S {
function(uint) external fn_uint;
}
contract C {
S storageStruct;
function test(S calldata calldataStruct) public returns (bool) {
storageStruct = calldataStruct;
return true;
}
}
// ----
// test((function)): "01234567890123456789abcd" -> true
// test((function)): 0x3031323334353637383930313233343536373839616263640000000000000001 -> FAILURE
// test((function)): 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff -> FAILUREYou can call this one copy_from_calldata_to_storage_external_function_type_member_invalid.sol.
Tests that 8 bytes of padding following the 24 byte address is zero, otherwise should revert (which it does, I ran it locally :)
Push this and I'll approve and merge.
7fba57e to
1f287aa
Compare
|
@nikola-matic added your suggested test. |
| <?dynamicallyEncodedMember> | ||
| let <memberValues> := <accessCalldataTail>(value, memberSrcPtr) | ||
| <!dynamicallyEncodedMember> | ||
| <?isValueType> | ||
| let <memberValues> := <read>(memberSrcPtr) | ||
| <!isValueType> | ||
| let <memberValues> := memberSrcPtr | ||
| </isValueType> | ||
| </dynamicallyEncodedMember> |
There was a problem hiding this comment.
This introduces a silent assumption that value types are never dynamically encoded. I think that's true now, but such types are technically possible in the encoding - it would be a case where you have an offset in the head and the value is fixed-size, but still stored in the tail.
No need to handle that, but please at least explicitly assert that dynamicallyEncodedMember && isValueType is impossible.
There was a problem hiding this comment.
Right.
Though before, in that case, it would default to the one read method for value types, possibly generating a miscompilation, I guess.
Added an assertion in the form: p -> q (= !p v q), with p: dynamically encoded, q: not value type.
…unction member to storage
…r from calldata to storage
1f287aa to
1c0f0e1
Compare
fix #16717.