Check offset overflow in fd_pwrite

[yagehu]

This commit fixes a potential overflow in fd_pwrite. Since uv_fs_write takes an int64_t as the offset while fd_pwrite accepts an uint64_t, we need to check it doesn't overflow when cast.

[guybedford]

Strictly speaking, this is not actually an overflow though I believe because Wasm treats u64 types as i64 types in the host integration layer since it doesn't distinguish between them. Thus while to native callers it is taking an i64, negative values passed should still be interpreted as their unsigned equivalents. See https://webassembly.github.io/spec/core/syntax/types.html#number-types for more info on this from a Wasm perspective.

[guybedford]

That said, ideally uvwasi would be properly typed here in having a signature that takes a uint64_t and not a signature that takes a int64_t to begin with.

[guybedford]

But we should not land this overflow check as the correct interpretation of the code is as an unsigned argument.

[yagehu]

Hi @guybedford I created an issue #256 explaining the motivation. I don't think this statement:

negative values passed should still be interpreted as their unsigned equivalents

applies to the uv_fs_write call as it is an implementation detail, not spec'd by Wasm.

[mcollina]

lgtm

[guybedford]

Here is the preview1 spec for fd_pwrite - https://github.com/WebAssembly/WASI/blob/main/legacy/preview1/docs.md#fd_pwrite.

offset is a filesize (https://github.com/WebAssembly/WASI/blob/main/legacy/preview1/docs.md#filesize), which has type u64.

Therefore the interface is not correct by the spec.

Implementation behaviour is another question of course, and we can choose to ban this, but my clarification is that this is valid per the preview1-specified function signature.

[guybedford]

On second read I see what you mean about the implementation detail here.

That the function signature is not u64 is actually a separate question I suppose then, regardless of implementation bounds per this PR.

Thanks for talking it through.

[sunfishcode]

The proposed fix looks good for now, and I've now filed WebAssembly/wasi-filesystem#146 to discuss what should do in the spec.