fix(interpreter): Stack push_slice fix and dup with pointers (#837)

[DaniPopes]

• get_unchecked{,_mut} is always UB if out of bounds. This is easy to overlook because it works correctly, but it's still UB. Use pointers instead.
• Stack::push_slice (refactor: rewrite Stack::push_slice to allow arbitrary lengths #812) did not handle more than one word (32 bytes) correctly. Added simple unit tests to verify correct behavior. No performance changes.

[rakita]

• get_unchecked{,_mut} is always UB if out of bounds. This is easy to overlook because it works correctly, but it's still UB. Use pointers instead.

The pointer would have the same UB behaviour, right? This is safe as to as the bounds are checked and we know the capacity of data vec.

[DaniPopes]

No because get_unchecked* returns a reference, which has way more requirements for validity than pointers.

get_unchecked docs say:

Calling this method with an out-of-bounds index is undefined behavior even if the resulting reference is not used.

Miri reports UB in Stack::dup on main:

[rakita]

No because get_unchecked* returns a reference, which has way more requirements for validity than pointers.

get_unchecked docs say:

Calling this method with an out-of-bounds index is undefined behavior even if the resulting reference is not used.

Miri reports UB in Stack::dup on main:

That seems like a false positive, reference is just a sugar here.

It seems that Vec uses get_unchecked_mut from the slice that works on slice len so that is probably of reason for false positive as it does not know the capacity of the vector.

So just moving self.data.set_len(len + 1); above should fix miri

[DaniPopes]

It's still uninitialized memory, so a reference to that is always undefined behavior. It's not a false positive. Moving set_len might silence miri but I don't think it fixes the UB.

"it works" because we have checked the bounds of the stack allocation and it happens that the implementation detail for get_unchecked works out of bounds too (i think), but it's still out of bounds so it shouldn't be relied upon.

[rakita]

It's still uninitialized memory, so a reference to that is always undefined behavior. It's not a false positive. Moving set_len might silence miri but I don't think it fixes the UB.

"it works" because we have checked the bounds of the stack allocation and it happens that the implementation detail for get_unchecked works out of bounds too (i think), but it's still out of bounds so it shouldn't be relied upon.

You are right, uninit memory should be dealt with, I have spent a lot of time in cpp land so this doesn't trigger me. Uninit can be a big problem and UB when you have a type that does the Drop or possibly the padding, for U256 this is not the case so it is considered safe here and not a UB.

Good read here for exactly this: rust-lang/rust-clippy#4483
It seems MaybeUninit was made to address this: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html

[rakita]

lgtm