napi_get_value_uint32 difference between Node 14 and previous

[emilbayes]

Hi,

In sodium-native we have a special malloc that allows you to allocated a mmap'ed buffer with special flags that provide extra protection. The signature is sodium_malloc(bytes) where bytes can be up to 0xffffffff. Until Node 14 napi_get_value_uint32 would fail for numbers outside this range (eg. -1 or Number.MAX_SAFE_INTEGER) but for Node 14 our tests now fail with varying errors on different operating system. My guess is that in Node 14, this wraps around, but it is unspecified by the napi docs whether wrap around does happen, so we might have relied on undefined behaviour.

Here is our failing tests:
https://github.com/sodium-friends/sodium-native/blob/91e28b44648164ba8054f22e9a96c826329462bd/test/memory.js#L117-L125

Here is the macro we use to read a uint32_t from a Number:
https://github.com/sodium-friends/sodium-native/blob/91e28b44648164ba8054f22e9a96c826329462bd/macros.h#L157-L161

[addaleax]

Until Node 14 napi_get_value_uint32 would fail for numbers outside this range (eg. -1 or Number.MAX_SAFE_INTEGER)

@emilbayes Are you sure that that is the case? The code in N-API hasn’t changed here, and it has always implemented wrap-around behavior for e.g. negative numbers (i.e. it implements ToUint32 from the JS spec).

[emilbayes]

No, we could have been doing the wrong thing all along 🤔

Just saw that it started breaking on Node 14: https://travis-ci.org/github/sodium-friends/sodium-native/builds/680466769

[gabrielschulhof]

@emilbayes I checked all the way back to v10.x and napi_get_value_uint32() of -1 always returns 0xffffffff.

[gabrielschulhof]

@emilbayes I have added a test to ensure that the behaviour remains consistent going forward. Please re-open if you believe that the problem lies within Node.js.