Return uint32 from WASI functions #2749
Conversation
Returning uint16 from WASI functions is technically correct. However, the smallest integer type in WASM is int32 and since we don't guarantee that the upper 16 bits of the result are zero'ed, it can result in tricky bugs if the language SDK being used in the WASM app does not cast back immediately to uint16. To prevent this, we directly return uint32 instead, so that the result is well-defined as a 32-bit number.
zoraaver
force-pushed
the
fix-errno-size
branch
from
1ad2d77
to
ae5eee2
Compare
| // contains garbage from the WASM app perspective. To prevent this, we return | ||
| // uint32 directly instead so as not to be reliant on the correct behaviour of | ||
| // any current/future WASI SDK implemenations. | ||
| typedef uint32_t wasi_errno_t; |
There was a problem hiding this comment.
Is it better to change __wasi_errno_t to uint32_t?
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/shared/platform/include/platform_wasi_types.h#L77
There was a problem hiding this comment.
I didn't want to change __wasi_errno_t since it's used in one of the WASI datatypes, https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/shared/platform/include/platform_wasi_types.h#L311-L323 (meaning the struct would become incompatible with the WASI ABI). We could change __wasi_event_t to use uint16 directly for the error field but I think it's a bit less confusing if we keep the internal error type consistent with the ABI and modify wasi_errno_t which is only used for the return type of WASI functions.
There was a problem hiding this comment.
OK, so let's merge this PR.
Returning uint16 from WASI functions is technically correct. However, the smallest integer type in WASM is int32 and since we don't guarantee that the upper 16 bits of the result are zero'ed, it can result in tricky bugs if the language SDK being used in the WASM app does not cast back immediately to uint16. To prevent this, we directly return uint32 instead, so that the result is well-defined as a 32-bit number.
Returning uint16 from WASI functions is technically correct. However, the smallest integer type in WASM is int32 and since we don't guarantee that the upper 16 bits of the result are zero'ed, it can result in tricky bugs if the language SDK being used in the WASM app does not cast back immediately to uint16. To prevent this, we directly return uint32 instead, so that the result is well-defined as a 32-bit number.
I originally noticed this issue when building WAMR with clangcl on Windows and running a rust WASM app but there's no reason it wouldn't affect WAMR on other platforms built with different toolchains. I'll look to fix the rust SDK as well but this is still probably a good idea in case some other SDKs are not handling the error result correctly (i.e. casting to uint16).