n-api: throw RangeError in napi_create_dataview() with invalid range #17869

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

+                           "napi_create_dataview",
+                           "byte_offset + byte_length should be less than or "
+                           "equal to the size in bytes of the array passed in");
+  }


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

+  if (byte_length + byte_offset > buffer->ByteLength()) {
+    return napi_throw_range_error(
+        env,
+        "napi_create_dataview",


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

@@ -433,6 +433,8 @@ E('ERR_MODULE_RESOLUTION_LEGACY', '%s not found by import in %s.' +
 E('ERR_MULTIPLE_CALLBACK', 'Callback called multiple times');
 E('ERR_NAPI_CONS_FUNCTION', 'Constructor must be a function');
 E('ERR_NAPI_CONS_PROTOTYPE_OBJECT', 'Constructor.prototype must be an object');
+E('ERR_NAPI_INVALID_DATAVIEW_ARGS',
+  'Invalid arguments were passed to napi_create_dataview()');


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: throw RangeError in napi_create_dataview() with invalid range #17869

n-api: throw RangeError in napi_create_dataview() with invalid range #17869

romandev commented Dec 26, 2017

addaleax Dec 27, 2017

romandev Dec 27, 2017

TimothyGu left a comment

romandev commented Dec 27, 2017

romandev commented Jan 2, 2018

mhdawson Jan 3, 2018

romandev Jan 3, 2018

mhdawson commented Jan 3, 2018

mhdawson Jan 3, 2018

mhdawson Jan 3, 2018

romandev Jan 4, 2018

mhdawson left a comment

mhdawson commented Jan 4, 2018

romandev commented Jan 5, 2018 •

edited

Loading

romandev Jan 5, 2018

mhdawson commented Jan 5, 2018

jasnell commented Jan 10, 2018

n-api: throw RangeError in napi_create_dataview() with invalid range #17869

n-api: throw RangeError in napi_create_dataview() with invalid range #17869

Conversation

romandev commented Dec 26, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

romandev commented Dec 27, 2017

romandev commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jan 3, 2018

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 4, 2018

romandev commented Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

mhdawson commented Jan 5, 2018

jasnell commented Jan 10, 2018

romandev commented Jan 5, 2018 •

edited

Loading