n-api: throw RangeError in napi_create_dataview() with invalid range#17869
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ src/node_api.cc Outdated
There was a problem hiding this comment.
Shouldn't there be some kind of early return in this case? Otherwise we still create the DataView instance & pass it in *result... right?
There was a problem hiding this comment.
You're right. I missed return statement. :(
Done. Thank you for review.
romandevforce-pushed the dataview_range branch from 594dcf2 to 7bbd2ffCompare
TimothyGu left a comment
TimothyGu left a comment There was a problem hiding this comment.
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 commented Dec 27, 2017
Thank you for review. You're right. In JS code, [1] https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-dataview.cc?sq=package:chromium&l=74 |
romandev commented Jan 2, 2018
Gentle ping :) |
TimothyGu added the author ready 
src/node_api.cc Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thank you for review. Done.
mhdawson commented Jan 3, 2018
Just one comment about how we define the error code but after that it fixed up it looks good. |
lib/internal/errors.js Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I should have suggested that in the original comment.
There was a problem hiding this comment.
Thank you for explanation.
I've addressed your comment in my latest patch.
mhdawson commented Jan 4, 2018
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
@mhdawson I've just fixed the CI error in latest patch. Also, I tested it on my Mac and Linux. (Used this command |
| "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"); | ||
| returnnapi_set_last_error(env, napi_pending_exception); |
There was a problem hiding this comment.
FYI, a crash occurred because there was no this line.
mhdawson commented Jan 5, 2018
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
jasnell commented Jan 10, 2018
Landed in 91c1ccd |
TimothyGu removed the author ready 
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
7 participants
The API is required that
byte_length + byte_offsetis less than orequal 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), orvcbuild test(Windows) passesAffected core subsystem(s)
n-api