@memcpy panics on shared-memory WASM

[Aandreba]

Zig Version

0.11.0-dev.3332+76aa1fffb

Steps to Reproduce and Observed Behavior

The bug was found when executing a WASM library in Deno

Try creating an ArrayList, and then reserve capacity with ensureTotalCapacityPrecise.

Here is the code, but whith ensureTotalCapacityPrecise extracted

var alpha = std.ArrayList(u8).init(alloc);
{
    const T = u8;
    const alignment = null;
    var self = alpha;
    const new_capacity = 10;

    if (self.capacity >= new_capacity) return;

    const old_memory = self.allocatedSlice();
    if (self.allocator.resize(old_memory, new_capacity)) {
        self.capacity = new_capacity;
    } else {
        const new_memory = try self.allocator.alignedAlloc(T, alignment, new_capacity);
        // In this line, the code panics throwing `RuntimeError: memory access out of bounds` in Deno
        @memcpy(new_memory[0..self.items.len], self.items);
        self.allocator.free(old_memory);
        self.items.ptr = new_memory.ptr;
        self.capacity = new_memory.len;
    }
}

Replace @memcpy with std.mem.copyForwards, and the code stops panicking.

var alpha = std.ArrayList(u8).init(alloc);
{
    const T = u8;
    const alignment = null;
    var self = alpha;
    const new_capacity = 10;

    if (self.capacity >= new_capacity) return;

    const old_memory = self.allocatedSlice();
    if (self.allocator.resize(old_memory, new_capacity)) {
        self.capacity = new_capacity;
    } else {
        const new_memory = try self.allocator.alignedAlloc(T, alignment, new_capacity);
        // Now this line executes as expected
        std.mem.copyForwards(u8, new_memory[0..self.items.len], self.items);
        self.allocator.free(old_memory);
        self.items.ptr = new_memory.ptr;
        self.capacity = new_memory.len;
    }
}

Expected Behavior

Well, this shoudn't panic!

My theory is that there is something about Wasm shared memory that messes up @memcpy when doing zero-sized copies (It seems to work fine when actually copying values, so it may have to do with the high value of the original undefined pointer). Apart from that, I have no idea,

[Luukdegram]

This isn't caused by enabling shared-memory, this is due to enabling bulk_memory feature (which is required for shared-memory). This feature allows to usage of the mem.cpy instruction, whereas without this feature a loop will be emit instead. Zero-length slices have their ptr field set to undefined, which essentially means the address it's trying to copy from is 0xaaaaaaaa and may be out of bounds. This doesn't cause a runtime panic while not using the bulk-memory feature, because it uses a loop that won't be executed due to the len field of the slice being set to 0.

@andrewrk I can't really find the best solution here, we could perhaps add a safety check for the WebAssembly targets to check for zero-lengths. I don't think changing the value of the ptr field to anything else than undefined is correct either. Perhaps memcpy'ing a zero-length slice is considered UB? In this case, this panic would be considered the right behavior.

[Aandreba]

Perhaps memcpy'ing a zero-length slice is considered UB? In this case, this panic would be considered the right behavior.

But if this is done, changes will have to be done to ArrayList to manually avoid using memcpy on zero-sized slices (and I reckon a lot of other parts of the standard library will have to be updated too).

I think that a check for zero-sized copies on Wasm is the best option (that I can think of, at least).

[andrewrk]

The language is correct to allow zero length memcpy with undefined pointer value, even when the target is webassembly. It is each backend's job to make it work when lowering.

This is arguably a bug in the wasm runtime or even the specification. Getting those third parties to change their behavior is the first path we should explore.

Failing that, there could be a "CPU feature" flag that indicates the wasm runtime can't ignore the address of a length zero memcpy, which the wasm backend can branch on and emit checks for length zero if necessary.

[nektro]

Perhaps memcpy'ing a zero-length slice is considered UB?

it is in libc which runtimes may be using

still trying to find a spec reference to double check how wasm defines it

[andrewrk]

it is in libc

citation needed

[nektro]

https://man7.org/linux/man-pages/man3/memcpy.3.html and https://linux.die.net/man/3/memcpy are very sparse and do not describe nearly any error conditions explicitly

that reasoning was taken from https://en.cppreference.com/w/cpp/string/byte/memcpy which has the line:

If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero.

[andrewrk]

Note that is the C++ reference. The C one is here: https://en.cppreference.com/w/c/string/byte/memcpy
and it is worded a lot differently.

Regardless, Zig is a different language than C, and I am quite certain that len=0 will always be well-defined behavior, regardless of the pointer values of src and dest.

[Luukdegram]

still trying to find a spec reference to double check how wasm defines it

You can find that in the overview in the original proposal:
https://github.com/WebAssembly/bulk-memory-operations/blob/master/proposals/bulk-memory-operations/Overview.md#memorycopy-instruction

A trap occurs if:
the source offset plus size is greater than the length of the source memory
the destination offset plus size is greater than the length of the target memory
The bounds check is performed before any data are written.

It seems that originally it wasn't bound-checked for zero-length, but that was ultimately reverted. Some discussions around this can be found in WebAssembly/bulk-memory-operations#111 and WebAssembly/bulk-memory-operations#124 the main reasoning being simplicity and runtime cost for the runtime itself.

Considering it's part of the spec, we can only attempt to have them change the specification with the option 2 being a change to emit a check in the LLVM and Wasm backend for the memcpy instruction but only when cpu-feature bulk-memory is enabled.