unsafe_wild_copy is undefined behaviour

[pascalkuthe]

the trick used in unsafe_wild_copy is undefined behavior in the rust memory model.
In particular it violates the following rule from https://doc.rust-lang.org/std/ptr/index.html#safety (emphasis is mine):

For a pointer to be valid, it is necessary, but not always sufficient, that the pointer be dereferenceable: the memory range of the given size starting at the pointer must all be within the bounds of a single allocated object+. Note that in Rust, every (stack-allocated) variable is considered a separate allocated object.

The trick you linked is well known but it's simply not something that rusts memory model ever permits. That is also the reason both miri and the memory sanitizer complains (which were disabled). Even if it happens to work right now it can lead to miss-compilations in a new compiler version or even if just compiling a different project.

You may be able to implement this with inline-assembly instead (since you are not bound by the rules of the rust abstract machine there) but I would seek clarification for that from the rust project first too. As this crate has seen quite a bit of usage and features prominently on the serialization benchmark I am considering filing an rustsec advisory. I wanted to give a headsup first

[finnbear]

Hello, thanks for pointing this out. The code was intentionally unsound for a considerable increase to the performance of encoding Vec and ArrayVec (see #51 for a few benchmarks). We acknowledged this on the Security page in the Wiki, here.

You may be able to implement this with inline-assembly instead

I previously successfully implemented an older version of this trick, which only copied up to 8 bytes, in inline-assembly. The current version may copy more bytes, so the assembly would require a loop or manually-chosen SIMD instruction(s).

I am considering filing an rustsec advisory

An informational soundness advisory would be valid, for bitcode 0.6.0-alpha.1 - 0.6.3 inclusive on x86, x86_64, aarch64 in release mode. We may do something, possibly #51, such that 0.6.4 doesn't do UB by default.

[pascalkuthe]

Thanks for the fast response. I would be very uncomfortable with any dependency that relies on undefined behavior (pretty much the norm in the rust ecosystem) even in not security critical applications. Putting it behind a feature flag that is disabled by default makes sense. That kind of thing shouldn't be enabled by default in a library meant for wide consumption.

If you reimplement this in assembly then you would indeed have to essentially write memcopy in assembly. May be feasible if the sizes are somewhat constrainted tk a couple fized small values but would be quite painful otherwise.