n-api: throw RangeError in napi_create_typedarray() with invalid args #18037

romandev · 2018-01-08T14:43:08Z

According to the ECMA spec, we should throw a RangeError in the
following cases:

(length * elementSize) + offset > the size of the array passed in
offset % elementSize != 0

In the current implementation, this check was omitted. So, the following
code will cause a crash.

  napi_create_typedarray(env, napi_uint16_array, 2 /* length */,
                         buffer, 1 /* byte_offset */, &output_array);

This change fixes the problem and write some related tests.

Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

n-api

romandev

@mhdawson (or others) PTAL

cjihrig

LGTM with a few nits.

cjihrig · 2018-01-08T15:37:34Z

doc/api/errors.md

+<a id="ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT"></a>
+### ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT
+
+While calling `napi_create_typedarray()`, a given `offset` was not a multiple of


Maybe change "a given" to "the provided"?

cjihrig · 2018-01-08T15:38:30Z

doc/api/errors.md

+### ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT
+
+While calling `napi_create_typedarray()`, a given `offset` was not a multiple of
+size of element of given type.


Maybe change "size of element of given type" to "the element size"?

cjihrig · 2018-01-08T15:39:40Z

doc/api/errors.md

+### ERR_NAPI_INVALID_TYPEDARRAY_LENGTH
+
+While calling `napi_create_typedarray()`, `(length * size_of_element) +
+byte_offset` was larger than a length of given `buffer`.


"a length of given" -> "the length of"

cjihrig · 2018-01-08T15:43:44Z

test/addons-napi/test_typedarray/test.js

+  const template = Reflect.construct(currentType, buffer);
+  assert.throws(() => {
+    test_typedarray.CreateTypedArray(template, buffer, 0, 129);
+  }, RangeError);


Instead of just checking for RangeError, can you also validate the error message.

romandev

Thank you for review.
I addressed all your comments.

Another look?

romandev · 2018-01-08T16:50:25Z

doc/api/errors.md

+<a id="ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT"></a>
+### ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT
+
+While calling `napi_create_typedarray()`, a given `offset` was not a multiple of


romandev · 2018-01-08T16:50:33Z

doc/api/errors.md

+### ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT
+
+While calling `napi_create_typedarray()`, a given `offset` was not a multiple of
+size of element of given type.


romandev · 2018-01-08T16:50:39Z

doc/api/errors.md

+### ERR_NAPI_INVALID_TYPEDARRAY_LENGTH
+
+While calling `napi_create_typedarray()`, `(length * size_of_element) +
+byte_offset` was larger than a length of given `buffer`.


romandev · 2018-01-08T16:50:47Z

test/addons-napi/test_typedarray/test.js

+  const template = Reflect.construct(currentType, buffer);
+  assert.throws(() => {
+    test_typedarray.CreateTypedArray(template, buffer, 0, 129);
+  }, RangeError);


mhdawson

LGTM

mhdawson · 2018-01-08T23:14:48Z

Will leave a bit more time for comments and then kick of CI run tomorrow.

mhdawson · 2018-01-09T15:30:08Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/12461/

mhdawson · 2018-01-10T16:59:21Z

@romandev either the test does not handle big endian or we have a bug as one of the tests failed on all of the Big Endian platforms:

https://ci.nodejs.org/job/node-test-commit-plinux/14443/nodes=ppcbe-ubuntu1404/console

not ok 1876 addons-napi/test_typedarray/test

duration_ms: 0.173
severity: fail
stack: |-
assert.js:288
throw actual;
^

RangeError [ERR_NAPI_INVALID_TYPEDARRAY_LENGTH]: Invalid typed array length
    at assert.throws (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcbe-ubuntu1404/test/addons-napi/test_typedarray/test.js:71:21)
    at getActual (assert.js:252:5)
    at Function.throws (assert.js:260:18)
    at nonByteArrayTypes.forEach (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcbe-ubuntu1404/test/addons-napi/test_typedarray/test.js:70:10)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcbe-ubuntu1404/test/addons-napi/test_typedarray/test.js:68:19)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:577:32)
    at tryModuleLoad (module.js:517:12)

...

romandev · 2018-01-11T12:46:17Z

Should I use virtual machine to test on the platforms? or is there a better way?

mhdawson · 2018-01-11T22:00:02Z

@jBarz can you take a quick look at the change/code to see if you can easily suggest a fix ? @romandev if @jBarz can't come up with a suggestion easily we can request access to one of the community BE machines so you can investigate.

jBarz · 2018-01-12T02:59:29Z

test/addons-napi/test_typedarray/test_typedarray.c

+    NAPI_ASSERT(env, valuetype2 == napi_number,
+        "Wrong type of arguments. Expects a number as third argument.");
+
+    NAPI_CALL(env, napi_get_value_uint32(env, args[2], (uint32_t*)(&length)));


The cast won't work on big-endian. So something like this maybe?

uint32_t locallength = length; NAPI_CALL(..., &locallength); length = locallength;

Thank you for input. Done.

jBarz · 2018-01-12T03:00:01Z

test/addons-napi/test_typedarray/test_typedarray.c

+        "Wrong type of arguments. Expects a number as third argument.");
+
+    NAPI_CALL(env, napi_get_value_uint32(
+        env, args[3], (uint32_t*)(&byte_offset)));


jBarz

I made some comments about some explicit pointer casts that might be causing the big endian issues.

According to the ECMA spec, we should throw a RangeError in the following cases: - `(length * elementSize) + offset` > the size of the array passed in - `offset % elementSize` != `0` In the current implementation, this check was omitted. So, the following code will cause a crash. ``` napi_create_typedarray(env, napi_uint16_array, 2 /* length */, buffer, 1 /* byte_offset */, &output_array); ``` This change fixes the problem and write some related tests. Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length

mhdawson · 2018-01-15T18:39:58Z

New CI run: https://ci.nodejs.org/job/node-test-pull-request/12546/

romandev · 2018-01-16T01:33:06Z

@mhdawson Thank you for triggering CI.
It looks bot failure but it's not related to this PR.

sequential/test-timers-set-interval-excludes-callback-duration (failure)

mhdawson · 2018-01-16T20:17:04Z

CI failure is known issue -> #18160 and not related. CI looks clean.

According to the ECMA spec, we should throw a RangeError in the following cases: - `(length * elementSize) + offset` > the size of the array passed in - `offset % elementSize` != `0` In the current implementation, this check was omitted. So, the following code will cause a crash. ``` napi_create_typedarray(env, napi_uint16_array, 2 /* length */, buffer, 1 /* byte_offset */, &output_array); ``` This change fixes the problem and write some related tests. Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length PR-URL: #18037 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

mhdawson · 2018-01-16T20:24:35Z

Landed as 316b5ef

According to the ECMA spec, we should throw a RangeError in the following cases: - `(length * elementSize) + offset` > the size of the array passed in - `offset % elementSize` != `0` In the current implementation, this check was omitted. So, the following code will cause a crash. ``` napi_create_typedarray(env, napi_uint16_array, 2 /* length */, buffer, 1 /* byte_offset */, &output_array); ``` This change fixes the problem and write some related tests. Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length PR-URL: #18037 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins · 2018-02-27T05:42:41Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

According to the ECMA spec, we should throw a RangeError in the following cases: - `(length * elementSize) + offset` > the size of the array passed in - `offset % elementSize` != `0` In the current implementation, this check was omitted. So, the following code will cause a crash. ``` napi_create_typedarray(env, napi_uint16_array, 2 /* length */, buffer, 1 /* byte_offset */, &output_array); ``` This change fixes the problem and write some related tests. Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length PR-URL: nodejs#18037 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

According to the ECMA spec, we should throw a RangeError in the following cases: - `(length * elementSize) + offset` > the size of the array passed in - `offset % elementSize` != `0` In the current implementation, this check was omitted. So, the following code will cause a crash. ``` napi_create_typedarray(env, napi_uint16_array, 2 /* length */, buffer, 1 /* byte_offset */, &output_array); ``` This change fixes the problem and write some related tests. Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length Backport-PR-URL: #19447 PR-URL: #18037 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

According to the ECMA spec, we should throw a RangeError in the following cases: - `(length * elementSize) + offset` > the size of the array passed in - `offset % elementSize` != `0` In the current implementation, this check was omitted. So, the following code will cause a crash. ``` napi_create_typedarray(env, napi_uint16_array, 2 /* length */, buffer, 1 /* byte_offset */, &output_array); ``` This change fixes the problem and write some related tests. Refs: https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length Backport-PR-URL: #19265 PR-URL: #18037 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 8, 2018

romandev commented Jan 8, 2018

View reviewed changes

cjihrig approved these changes Jan 8, 2018

View reviewed changes

jasnell approved these changes Jan 8, 2018

View reviewed changes

romandev force-pushed the typedarray branch 2 times, most recently from 3677e16 to 48503c6 Compare January 8, 2018 16:52

romandev commented Jan 8, 2018

View reviewed changes

mhdawson approved these changes Jan 8, 2018

View reviewed changes

jBarz reviewed Jan 12, 2018

View reviewed changes

jBarz suggested changes Jan 12, 2018

View reviewed changes

romandev force-pushed the typedarray branch from 48503c6 to 3e6d561 Compare January 14, 2018 01:32

mhdawson closed this Jan 16, 2018

joyeecheung mentioned this pull request Jan 18, 2018

Investigate flaky sequential/test-async-wrap-getasyncid #18190

Closed

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added dont-land-on-v6.x labels Feb 27, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: throw RangeError in napi_create_typedarray() with invalid args #18037

n-api: throw RangeError in napi_create_typedarray() with invalid args #18037

romandev commented Jan 8, 2018 •

edited

Loading

romandev left a comment

cjihrig left a comment

cjihrig Jan 8, 2018

romandev Jan 8, 2018

cjihrig Jan 8, 2018

romandev Jan 8, 2018

cjihrig Jan 8, 2018

romandev Jan 8, 2018

cjihrig Jan 8, 2018

romandev Jan 8, 2018

romandev left a comment

romandev Jan 8, 2018

romandev Jan 8, 2018

romandev Jan 8, 2018

romandev Jan 8, 2018

mhdawson left a comment

mhdawson commented Jan 8, 2018

mhdawson commented Jan 9, 2018

mhdawson commented Jan 10, 2018

romandev commented Jan 11, 2018

mhdawson commented Jan 11, 2018

jBarz Jan 12, 2018

romandev Jan 14, 2018

jBarz Jan 12, 2018

jBarz left a comment •

edited

Loading

mhdawson commented Jan 15, 2018

romandev commented Jan 16, 2018

mhdawson commented Jan 16, 2018

mhdawson commented Jan 16, 2018

MylesBorins commented Feb 27, 2018

n-api: throw RangeError in napi_create_typedarray() with invalid args #18037

n-api: throw RangeError in napi_create_typedarray() with invalid args #18037

Conversation

romandev commented Jan 8, 2018 • edited Loading

Checklist

Affected core subsystem(s)

romandev left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romandev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jan 8, 2018

mhdawson commented Jan 9, 2018

mhdawson commented Jan 10, 2018

not ok 1876 addons-napi/test_typedarray/test

romandev commented Jan 11, 2018

mhdawson commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jBarz left a comment • edited Loading

Choose a reason for hiding this comment

mhdawson commented Jan 15, 2018

romandev commented Jan 16, 2018

mhdawson commented Jan 16, 2018

mhdawson commented Jan 16, 2018

MylesBorins commented Feb 27, 2018

romandev commented Jan 8, 2018 •

edited

Loading

jBarz left a comment •

edited

Loading