Skip to content

Conversation

@wchargin
Copy link

@wchargin wchargin commented Jan 22, 2022

While new bytes(n) may always point to a word-aligned slot, a
bytestring can be advanced to an arbitrary location by inline assembly:

bytes memory b = bytes("hello world");
uint256 prefixLength = 6;
assembly {
    let newLength := sub(mload(b), prefixLength)
    b := add(b, prefixLength)
    mstore(b, newLength)
}
myContract.myMethod(b); // passes "world"

This works fine, because the first 32 bytes of b still represent its
length, and the next mload(b) bytes still hold the data. This patch
clarifies that it's explicitly allowed.

wchargin-branch: docs-bytes-unaligned-ok

@wchargin wchargin force-pushed the wchargin-docs-bytes-unaligned-ok branch from 46c3eb0 to dd387cb Compare January 22, 2022 05:38
While `new bytes(n)` may always point to a word-aligned slot, a
bytestring can be advanced to an arbitrary location by inline assembly:

```solidity
bytes memory b = bytes("hello world");
uint256 prefixLength = 6;
assembly {
    let newLength := sub(mload(b), prefixLength)
    b := add(b, prefixLength)
    mstore(b, newLength)
}
myContract.myMethod(b); // passes "world"
```

This works fine, because the first 32 bytes of `b` still represent its
length, and the next `mload(b)` bytes still hold the data. This patch
clarifies that it's explicitly allowed.

wchargin-branch: docs-bytes-unaligned-ok
wchargin-source: 44943f1161c2c1a37ca0454b65a60987f554a590
@wchargin wchargin force-pushed the wchargin-docs-bytes-unaligned-ok branch from dd387cb to f0f1759 Compare January 22, 2022 05:39
@ekpyron
Copy link
Collaborator

ekpyron commented Jan 25, 2022

While you can do this in inline assembly, you really shouldn't and it is not guaranteed to work in all cases. So if we want to change the documentation here, I'd rather suggest to do the opposite and clarify that doing something like this in inline assembly results in "undefined behaviour"...

@chriseth
Copy link
Contributor

chriseth commented Feb 3, 2022

I agree. The fact that the compiler uses 32-byte aligned values most of the time and pads with zeros is a security mechanism, but nobody should rely on it, so it may be best to not document it at all.

@wchargin
Copy link
Author

wchargin commented Feb 3, 2022

Thanks for your comments! My end goal is to pass a prefix of a
bytestring as an argument to an external method, then remove that prefix
from the bytestring (in a loop). Doing this in obvious way by changing
the pointer and the size header made sense to me and works in my tests.
Here's the code, in case the example is helpful. Could you please
clarify whether this is legal, or if not how else I should do this
without redundantly memcpying the whole prefix into new memory?

The fact that the compiler uses 32-byte aligned values most of the
time and pads with zeros is a security mechanism, but nobody should
rely on it, so it may be best to not document it at all.

Acknowledged, thanks, and I think that we agree---my intent wasn't so
much to document that the compiler often uses 32-byte aligned values,
but rather to document that the user is not required to do so.

@maurelian
Copy link
Contributor

The suggested edit would just confuse me if I came across it out of the context of this PR.
You can do anything with assembly. Documenting all possible assembly edge cases is not feasible.

@leonardoalt
Copy link

I agree with @maurelian that the new text feels rather confusing outside of the context of this PR, and assembly allows way too many edge cases.
Thanks for the PR @wchargin , but I think there's consensus to not go after documenting the unsafe edge cases.

@wchargin
Copy link
Author

Thank you for adding the memory safety docs in #12620! This goes a long
way toward the kinds of things that I was hoping to find specs for when
sending this PR.

@wchargin wchargin deleted the wchargin-docs-bytes-unaligned-ok branch March 10, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants