Correctly construct Region to uphold deallocation safety invariant

[aumetra]

Closes #2061

To cite from the comments of the patch:

Shrinking the buffer down to the length is important to uphold a safety invariant by the dealloc method.
Passing in a differing size into the dealloc layout is considered undefined behaviour.

See: https://doc.rust-lang.org/stable/alloc/alloc/trait.GlobalAlloc.html#safety-2

[webmaster128]

Yeah, I now understand why this fix is correct, thank you! However, this will create a pointless clone of the data in many cases. I think there are two potential other ways to address it:

1. Don't call build_region(&buffer) but create a region of the entire buffer capacity
2. We only have this problem when buffers are created by the contract and then destroyed by the VM (see fn call_raw<A, S, Q>). But the whole instance.deallocate(res_region_ptr)?;is pointless because we dropinstance` one line later. This is like cleaning the house before demolishing it.

[aumetra]

Don't call build_region(&buffer) but create a region of the entire buffer capacity

Since all of the logic is private, I felt free to update the API of build_region.
Instead of taking a raw byte slice, it now takes something that implements the new unsafe trait RegionSource which is, by default, implemented for &[u8] and Vec<u8>.

The trait exposes the function capacity which has to, as documented in the trait safety section, return the full capacity of the memory region.
I wanna change the trait a bit, but yeah, this is roughly what I envisioned.

And testing against the latest nightly compiler, it fixes it.

[aumetra]

The AsRef<[u8]> bound was very Vec<u8> centric since other types which might implement this API only point to a sub-region of the allocated memory with a different starting pointer and so on and so forth..

So this is a better API.

[webmaster128]

That's beautiful, thanks!

[webmaster128]

Which provenance rules are you referring to?

The whole memory module is wasm32 only, such that we know

• The memory is a Wasm linear memory where memory can be addressed by offsets
• usize is 32 bit long

[aumetra]

Yeah, makes sense, but it's just internally a bunch of hints for the compiler and miri.
It contains stuff like the address space, address, and some other metadata which a usize can technically not accurately represent since it's just a pointer somewhere in memory.

It's technically just an experiment but they also wanna explore if it makes sense for the compiler to enforce provenance at some point AFAIK.

https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance

But as the docs say

The following text is non-normative, insufficiently formal, and is an extremely strict interpretation of provenance. It’s ok if your code doesn’t strictly conform to it.

Just something that was on my mind and I commented in ¯\_(ツ)_/¯
We can remove the comment, too.

[webmaster128]

Thanks, all good, no worries. I was just not sure if there is anything practically relevant we need to be careful of.

[webmaster128]

Sweet! Could you add a CHANGELOG entry for this?

[webmaster128]

Thanks, all good, no worries. I was just not sure if there is anything practically relevant we need to be careful of.

[aumetra]

Damn it, I always forget the changelog..

[aumetra]

I was just not sure if there is anything practically relevant we need to be careful of

No, not really. It's just something they are experimenting with to give the compiler more information about raw pointers.
Because the more info compilers have about the code, the more assumptions they can make, the more optimization tricks they can do (prime example: Fortran)

[aumetra]

@Mergifyio backport release/1.5

[mergify]

backport release/1.5

✅ Backports have been created

• #2064 Correctly construct Region to uphold deallocation safety invariant (backport #2062) has been created for branch release/1.5 but encountered conflicts

[aumetra]

@Mergifyio backport release/2.0

[mergify]

backport release/2.0

✅ Backports have been created

• #2073 Correctly construct Region to uphold deallocation safety invariant (backport #2062) has been created for branch release/2.0 but encountered conflicts