Skip to content
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

Necessary buffer padding details should be documented #5

Open
jselbie opened this issue Oct 9, 2020 · 1 comment
Open

Necessary buffer padding details should be documented #5

jselbie opened this issue Oct 9, 2020 · 1 comment

Comments

@jselbie
Copy link

jselbie commented Oct 9, 2020

This statement in the docs could be much improved:

The buffer size must be chosen very carefully, if you are not sure about that choose your biggest guess.

I'd like to save everyone hours of debugging insanity.

Minimum buffer size is the summation of sizes of all serialized values and enough padding such that this size value is a multiple of 32. Or in other words, up to 31 bytes of padding is needed.

Don't allocate a buffer like this based solely on the summation of sizeOf results for each variable:

uint size = sizeOfBool()*3 + sizeOfUnit(32)*2 + sizeofAddress()*2;
bytes memory data = new bytes(size);

The above will lead to undefined behavior after serializing values into it, incorrect program behavior, and Truffle/Ganache will do even weirder stuff (freeze/crash).

Compute the size of the buffer to account for needed padding:

uint size = sizeOfBool()*3 + sizeOfUnit(32)*2 + sizeofAddress()*2;
if ((size % 32)  > 0) {
    size += 32 - (size % 32);
}
bytes memory data = new bytes(size);
@MC-AM
Copy link

MC-AM commented Mar 1, 2022

I think it's more likely that you've run into the same issue that I have in that I've been updating the wrong bytes in my buffer by choosing a bad offset value. Continuing your example, if the rest of your code looks something like this (per the README examples):

uint size = sizeOfBool()*3 + sizeOfUnit(32)*2 + sizeofAddress()*2;
bytes memory data = new bytes(size);

uint offset = size;

addressToBytes(offset, myAddress1, data);
offset -= sizeOfAddress();   // offset == sizeOfBool()*3 + sizeOfUnit(32)*2 + sizeofAddress()

...

boolToBytes(offset, true, data);
offset -= sizeOfBool();    // offset == sizeOfBool()

boolToBytes(offset, true, data);

what ends up happening is that by the end, with offset == 1 you're updating the wrong memory location. Keeping in mind that the first 32 bytes of an array are reserved for specifying the length of the array, if your offset is ever less than (32 + size of data to store), you're going to be effectively randomly changing the size of the array. In turn, this can cause all sorts of funky behaviour as now your byte array is corrupted and using it can lead to corruption of other parts of your memory.

As a result, padding the buffer so that its size is a multiple of 32 will only work some of the time and the real solution is to ensure that you're never updating the first 32 bytes of the bytes array i.e. in the above example, simply adding 32 to size would suffice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants