-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relax the definition of memory safety in the documentation. #15238
Conversation
8a9ea50
to
07a9920
Compare
|
||
- By the end of the assembly block, the free memory pointer at offset `0x40` is restored to a sane value (i.e. it is either | ||
restored to its original value or an increment of it due to a manual memory allocation), and the memory word at offset ``0x60`` | ||
is restored to a value of zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't messing with the zero slot be dangerous if the assembly accesses dynamic Solidity arrays that point at it due to not being explicitly initialized? Writing anything to it, even only for the duration of the block, not only changes their value, but potentially extends them to overlap the area reserved for variables moved to memory.
This seems pretty unsafe to me. And even if we'd rather just consider that a part of the internals that a user of the inline assembly should be aware of, I think that we should at least add a warning about it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that should definitely be made clearer (even though it really doesn't have anything to do with memory-safety in the defined sense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a paragraph about it - but yeah, we could also define the zero pointer as fully off limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I guess it's not unsafe by itself, unless the user actually reads or writes such an arrays in the block. Still, it can lead to accidental unsafe memory use very easily.
Also, now that I think of it, changing memory pointer could lead to a similar mistake - if it happens to point somewhere inside the reserved area and the user tries to "allocate" memory on their own. Though that one would be more obviously user error. But it could become less obvious and more likely in the future if we allow using global functions in inline assembly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I mean, this is why we didn't specifically declare this as safe so far. I'd still not necessarily say that we have to - but the fact is that people are doing it, so it might be better to be very clear about if and when this is valid. But yeah, we can also clarify this further and produce some dangerous examples and such.
But yeah, that's also why I already didn't add this to the main section, but to a separate "advanced" section :-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify how Solidity would use 0x60
inside of an assembly block to initialize a dynamic array? I have been under the impression that all memory management within an assembly block is explicitly performed by mload
, mstore
and mcopy
(or calls to identity precompile) operations and that Solidity does not support dynamic memory arrays. Is there a current situation where Solidity reads or writes values from memory in an assembly block without an explicit command?
It also seems odd to use 0x60
to get a zero value which would require PUSH1 0x60 MLOAD
(6 gas, 3 bytes of bytecode) versus PUSH1 0x00
(3 gas, 2 bytes of bytecode) or PUSH0
(2 gas, 1 byte of bytecode) depending on target EVM version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Michael Amadi sent me the explanation for 0x60
in a dev chat - 0x60
would be the stack value for the memory pointer location of a zero length array so that when the length is read it returns zero. That makes sense but the use of dynamic memory langauge is confusing since it's actually an uninitialized static length array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On your last question, from my understanding, solidity uses 0x60 as zero slot because dynamic values have to hold a stack value that points to somewhere in memory (the offset to the length's memory offset) and if it points to somewhere that holds 0x00 e.g 0x60, then it assumes the len is stores 0x00 bytes away which is still 0x60 and when it reads this len it returns 0 since 0x60 still holds 0 and so returns a 0 len dynamic type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think it should be considered memory safe to overwrite 0x60 if all dynamic memory types within call scope are initialized I.e assigned a value (0 length or not). From my understanding, it's when the types are not assigned to that they point to 0x60.
By initialized I mean assigned a value even if it's a zero length value since once assigned solidity allocates memory for it
// this is allocated memory
string memory A = "";
bytes memory B = new bytes(0);
uint256[] memory C = new uint256[](len);
// this points to 0x60
string memory A;
bytes memory B;
uint256[] memory C;
What are your thoughts on it?
Is it too prone to foot guns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify the concern here: Solidity will, of course, not initialize any dynamic array within an assembly block. But an existing array in memory may have 0x60
as a value, which you need to take into account when overwriting the value.
So consider
function f(bool c) public {
uint[] memory x;
if (c) {
x = ...;
}
assembly {
function hashUintArray(ptr) -> y {
let length := mload(ptr)
let dataArea := add(ptr, 32)
y := keccak256(dataArea, add(dataArea, mul(length, 32)))
}
mstore(0x60, ...)
mstore(0x40, hashUintArray(x))
revert(0x40, 0x40)
}
}
Now, whenver c
is false
, whatever you've written to 0x60
will be interpreted as uint
array in memory and hashed - which is not the contents of x
.
This has technically nothing to do with "memory-safety" in the narrow sense defined in the docs, which is concerned with not overwriting areas the compiler may use to move stack variables to memory if it runs out of stack space (which can happen for variables in the assembly block).
But @cameel pointed out that it's worth mentioning when talking about overwriting the zero value at 0x60
.
Even worse, if the assembly block doesn't terminate as in the example, any past and future accesses to any previously uninitialized memory array will read from whatever you wrote to 0x60
instead of being treated as empty array.
So, to cover all possible cases, you need to make sure that both:
- If the assembly block does not terminate, i.e. you move back to Solidity code, memory offset
0x60
needs to have a value of zero again at the end of the assembly block, otherwise behaviour is undefined. - During the assembly block while you have memory offset
0x60
overwritten, you need to be aware that, if you manually read from Solidity memory arrays yourself in that assembly block, you can get unintended results (arrays that should be empty can have arbitrary content depending on what you wrote to0x60
).
Co-authored-by: Moritz Hoffmann <clonker@gmail.com>
83d2906
to
2696cff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I'd probably put the warning at the end in a RST warning block, but I don't want into nitpicking about details now. The content itself seems sound.
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
This pull request is stale because it has been open for 14 days with no activity. |
Came up in https://forum.soliditylang.org/t/non-memory-safe-assembly/2368.
De-facto this is already safe given the stack-to-memory logic. It's probably reasonable to strongly commit to this, guarantee this to be safe and consider changing this a breaking change.