n-api: throw RangeError in napi_create_typedarray() with invalid args by romandev · Pull Request #18037 · nodejs/node · GitHub

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

@mhdawson (or others) PTAL

LGTM with a few nits.

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

Done

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

Done

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

Done

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

Done

Thank you for review.
I addressed all your comments.

Another look?

Done

LGTM

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

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

@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)

...

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

@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.

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.

Ditto

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

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

@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)

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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

n-api: throw RangeError in napi_create_typedarray() with invalid args by romandev · Pull Request #18037 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

n-api: throw RangeError in napi_create_typedarray() with invalid args#18037romandev wants to merge 1 commit intonodejs:masternodejs/node:masterfrom romandev:typedarrayromandev/node:typedarrayCopy head branch name to clipboard

Conversation

romandev commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

romandev left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romandev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 8, 2018

Uh oh!

mhdawson commented Jan 9, 2018

Uh oh!

mhdawson commented Jan 10, 2018

not ok 1876 addons-napi/test_typedarray/test

Uh oh!

romandev commented Jan 11, 2018

Uh oh!

mhdawson commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jBarz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 15, 2018

Uh oh!

romandev commented Jan 16, 2018

Uh oh!

mhdawson commented Jan 16, 2018

Uh oh!

mhdawson commented Jan 16, 2018

Uh oh!

MylesBorins commented Feb 27, 2018

Uh oh!

Reviewers

Assignees

n-api: throw RangeError in napi_create_typedarray() with invalid args#18037
romandev wants to merge 1 commit into
nodejs:masterfrom
romandev:typedarray

romandev commented Jan 8, 2018 •
edited
Loading

jBarz left a comment •
edited
Loading