n-api: throw RangeError in napi_create_dataview() with invalid range by romandev · Pull Request #17869 · nodejs/node

romandev · 2017-12-26T14:38:34Z

The API is required that byte_length + byte_offset is less than or
equal to the size in bytes of the array passed in. If not, a RangeError
exception is raised.

Refs: https://nodejs.org/api/n-api.html#n_api_napi_create_dataview

Checklist

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

Affected core subsystem(s)

n-api

addaleax · 2017-12-27T13:49:09Z

src/node_api.cc

Shouldn't there be some kind of early return in this case? Otherwise we still create the DataView instance & pass it in *result... right?

You're right. I missed return statement. :(
Done. Thank you for review.

TimothyGu

The fact that V8 isn’t throwing an error on cases like this sounds like a bug to me, as the JS DataView constructor does throw. But, this LGTM.

romandev · 2017-12-27T16:29:53Z

Thank you for review. You're right. In JS code, new DataView(new ArrayBuffer(10), 3, 20) will throw RangeError[1] but V8 public API is not.

[1] https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-dataview.cc?sq=package:chromium&l=74

romandev · 2018-01-02T11:04:48Z

Gentle ping :)

mhdawson · 2018-01-03T19:21:07Z

src/node_api.cc

The CODE passed in should be defined in https://github.com/nodejs/node/blob/master/lib/internal/errors.js right after ERR_NAPI_CONTS_PROTOTYPE_OBJECT, and I'd suggest it be ERR_NAPI_INVALID_DATAVIEW_ARGS

Thank you for review. Done.

mhdawson · 2018-01-03T19:21:48Z

Just one comment about how we define the error code but after that it fixed up it looks good.

mhdawson · 2018-01-03T22:25:26Z

lib/internal/errors.js

The message here should match the message in src/node_api.cc. There will be a better way to handle errors on the native side but for now I think we want to make the messages the same.

I should have suggested that in the original comment.

Thank you for explanation.
I've addressed your comment in my latest patch.

mhdawson

LGTM

mhdawson · 2018-01-04T22:32:21Z

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

The API is required that `byte_length + byte_offset` is less than or equal to the size in bytes of the array passed in. If not, a RangeError exception is raised[1]. [1] https://nodejs.org/api/n-api.html#n_api_napi_create_dataview

romandev · 2018-01-05T01:45:09Z

@mhdawson I've just fixed the CI error in latest patch. Also, I tested it on my Mac and Linux. (Used this command /usr/bin/python2.7 tools/test.py --mode=release -J addons-napi)
Could you please re-run CI?

romandev · 2018-01-05T01:51:57Z

src/node_api.cc

+        "ERR_NAPI_INVALID_DATAVIEW_ARGS",
+        "byte_offset + byte_length should be less than or "
+        "equal to the size in bytes of the array passed in");
+    return napi_set_last_error(env, napi_pending_exception);


FYI, a crash occurred because there was no this line.

mhdawson · 2018-01-05T16:51:11Z

new CI : https://ci.nodejs.org/job/node-test-pull-request/12421/

The API is required that `byte_length + byte_offset` is less than or equal to the size in bytes of the array passed in. If not, a RangeError exception is raised[1]. [1] https://nodejs.org/api/n-api.html#n_api_napi_create_dataview PR-URL: #17869 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2018-01-10T00:23:29Z

Landed in 91c1ccd

The API is required that `byte_length + byte_offset` is less than or equal to the size in bytes of the array passed in. If not, a RangeError exception is raised[1]. [1] https://nodejs.org/api/n-api.html#n_api_napi_create_dataview PR-URL: #17869 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The API is required that `byte_length + byte_offset` is less than or equal to the size in bytes of the array passed in. If not, a RangeError exception is raised[1]. [1] https://nodejs.org/api/n-api.html#n_api_napi_create_dataview PR-URL: nodejs#17869 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The API is required that `byte_length + byte_offset` is less than or equal to the size in bytes of the array passed in. If not, a RangeError exception is raised[1]. [1] https://nodejs.org/api/n-api.html#n_api_napi_create_dataview Backport-PR-URL: #19447 PR-URL: #17869 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The API is required that `byte_length + byte_offset` is less than or equal to the size in bytes of the array passed in. If not, a RangeError exception is raised[1]. [1] https://nodejs.org/api/n-api.html#n_api_napi_create_dataview Backport-PR-URL: #19265 PR-URL: #17869 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Dec 26, 2017

romandev mentioned this pull request Dec 26, 2017

Implement an initial implementation of DataView class nodejs/node-addon-api#205

Closed

addaleax reviewed Dec 27, 2017

View reviewed changes

romandev force-pushed the dataview_range branch from 594dcf2 to 7bbd2ff Compare December 27, 2017 13:59

TimothyGu approved these changes Dec 27, 2017

View reviewed changes

cjihrig approved these changes Dec 27, 2017

View reviewed changes

addaleax approved these changes Dec 27, 2017

View reviewed changes

romandev force-pushed the dataview_range branch from 7bbd2ff to abc4e1a Compare January 1, 2018 09:16

TimothyGu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2018

mhdawson reviewed Jan 3, 2018

View reviewed changes

romandev force-pushed the dataview_range branch from abc4e1a to d85f019 Compare January 3, 2018 20:35

mhdawson reviewed Jan 3, 2018

View reviewed changes

romandev force-pushed the dataview_range branch from d85f019 to 5fdd4ac Compare January 4, 2018 00:02

mhdawson approved these changes Jan 4, 2018

View reviewed changes

mhdawson self-assigned this Jan 4, 2018

romandev force-pushed the dataview_range branch from 5fdd4ac to 2e329e9 Compare January 5, 2018 01:43

romandev commented Jan 5, 2018

View reviewed changes

jasnell approved these changes Jan 10, 2018

View reviewed changes

jasnell closed this Jan 10, 2018

TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

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

Uh oh!

Conversation

romandev commented Dec 26, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

romandev commented Dec 27, 2017

Uh oh!

romandev commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 3, 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!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 4, 2018

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 5, 2018

Uh oh!

jasnell commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

romandev commented Jan 5, 2018 •

edited

Loading