search

nodejs / uvwasi

WASI syscall API built atop libuv
MIT License
222 stars 48 forks source link

Check offset overflow in fd_pwrite #254

Closed yagehu closed 3 months ago

yagehu commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

yagehu commented 3 months ago

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.

guybedford commented 3 months ago

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.

sunfishcode commented 3 months ago

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