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

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

Closed
wants to merge 1 commit into from

Conversation

romandev
Copy link
Contributor

@romandev romandev commented Jan 8, 2018

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

@nodejs-github-bot 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
Copy link
Contributor Author

@romandev romandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson (or others) PTAL

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few nits.

<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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

### ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT

While calling `napi_create_typedarray()`, a given `offset` was not a multiple of
size of element of given type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

### ERR_NAPI_INVALID_TYPEDARRAY_LENGTH

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

@romandev romandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for review.
I addressed all your comments.

Another look?

<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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

### ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT

While calling `napi_create_typedarray()`, a given `offset` was not a multiple of
size of element of given type.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

### ERR_NAPI_INVALID_TYPEDARRAY_LENGTH

While calling `napi_create_typedarray()`, `(length * size_of_element) +
byte_offset` was larger than a length of given `buffer`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

mhdawson commented Jan 8, 2018

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

@mhdawson
Copy link
Member

mhdawson commented Jan 9, 2018

@mhdawson
Copy link
Member

@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
Copy link
Contributor Author

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

@mhdawson
Copy link
Member

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

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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for input. Done.

"Wrong type of arguments. Expects a number as third argument.");

NAPI_CALL(env, napi_get_value_uint32(
env, args[3], (uint32_t*)(&byte_offset)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Contributor

@jBarz jBarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@romandev
Copy link
Contributor Author

@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
Copy link
Member

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

mhdawson pushed a commit that referenced this pull request Jan 16, 2018
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>
@mhdawson
Copy link
Member

Landed as 316b5ef

@mhdawson mhdawson closed this Jan 16, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
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>
@MylesBorins
Copy link
Contributor

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.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
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>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
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>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants