Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using "unsigned int" for node::Buffer::kMaxLength prevents V8 from supporting larger TypedArrays #31399

Closed
jakobkummerow opened this issue Jan 17, 2020 · 2 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@jakobkummerow
Copy link
Contributor

V8 developer here. We would like to allow larger TypedArrays than the current limit of 0x7FFFFFFF == 2³¹ - 1. Bumping the limit to more than 0xFFFFFFFF == 2³² - 1 currently makes the V8+Node integration build fail because Node uses an unsigned int for node::Buffer::kMaxLength: https://github.com/nodejs/node/blob/master/src/node_buffer.h#L32

Please update Node's code to use size_t for (typed) array lengths and indices so that V8 can allow TypedArrays with lengths >= 2**32 on 64-bit platforms.
(We are currently not planning any changes for 32-bit platforms, and also not for regular, non-typed JavaScript arrays.)

From a quick look, it seems most places are already using size_t; aside from the definition itself I can only see one call to Integer::NewFromUnsigned that would have to be replaced with Number::New in https://github.com/nodejs/node/blob/master/src/node_buffer.cc#L1181; so hopefully this will not be much work.

Buildbot output:
https://ci.chromium.org/p/node-ci/builders/try/node_ci_linux64_rel/b8891010098315519824
Compiler error:

FAILED: obj/node/node_base/udp_wrap.o
/b/s/w/ir/cache/goma/client/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF...(too long)
In file included from ../../node/src/udp_wrap.cc:24:
../../node/src/node_buffer.h:32:40: error: implicit conversion from 'const size_t' (aka 'const unsigned long') to 'const unsigned int' changes value from 4294967296 to 0 [-Werror,-Wconstant-conversion]
static const unsigned int kMaxLength = v8::TypedArray::kMaxLength;
~~~~~~~~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
@bnoordhuis bnoordhuis added the buffer Issues and PRs related to the buffer subsystem. label Jan 18, 2020
@bnoordhuis
Copy link
Member

This is going to need a little more consideration than just switching types. kMaxLength > 2**31-1 allows users to request file reads and writes > 2 GB but libuv currently doesn't handle those well, see libuv/libuv#1501.

(It's more of a platform issue than a libuv issue but libuv is where the platform differences will be taken care of.)

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jan 18, 2020
Change the type of `Buffer::kMaxLength` to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.

Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.

Fixes: nodejs#31399
Refs: libuv/libuv#1501
@jakobkummerow
Copy link
Contributor Author

Thanks for the quick fix!

Maintaining existing limits for I/O operations sounds reasonable.

If needed, you could also consider decoupling Node's Buffer limit from V8's TypedArray limit:

static const unsigned int kMaxLength = 0x7FFFFFFF;
static_assert(kMaxLength <= v8::TypedArray::kMaxLength);

@Trott Trott closed this as completed in 5005c3c Jan 21, 2020
codebytere pushed a commit that referenced this issue Feb 17, 2020
Change the type of `Buffer::kMaxLength` to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.

Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.

Fixes: #31399
Refs: libuv/libuv#1501

PR-URL: #31406
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants