n-api: add check for large strings by mhdawson · Pull Request #15611 · nodejs/node · GitHub

n-api uses size_t for the size of strings when specifying
string lengths. V8 only supports a size of int. Add
a check so that an error will be returned if the user
passes in a string with a size larger than will fit into
an int.

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

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int.

LGTM, but I'd personally like a better error than "Unknown failure"

@cjihrig I was on the fence about adding a new error code. We have a small set so far and was not sure we'd want to add a lot of individual ones and I don't think this particular one will be occur very often. If we think its important I'll add.

@mhdawson What about napi_invalid_arg? That sounds a bit better?

I had started with that but the string for it seemed wrong. Thinking about it more though I should probably just change the string as it does not match the name. The current string is:

"Invalid pointer passed as argument",

But I should probably just update that to:

"Invalid argument",

Unless I hear other better suggestions I'll update to use napi_invalid_arg and update the corresponding message for it.

Pushed change to address comments.

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

Seems like test failed on 32 bit machines, need to tweak.

Pushed change to fix test failure on 32 bit platforms.

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

Arms issues were infra problems but would like to see tests run/pass there so kicked off a new ci run: https://ci.nodejs.org/job/node-test-pull-request/10336/

Arm failures where:#15655

Windows failure was: #15558

CI looks good landing.

Landed as cec6e21

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int. PR-URL: #15611 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int. PR-URL: nodejs/node#15611 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

I am not sure why but I get this error on my local machine

=== release test ===                                                           
Path: addons-napi/test_string/test
assert.js:686
      throw actual;
      ^

TypeError: test_string.TestLargeUtf8 is not a function
    at assert.throws (/home/ruben/repos/node/node/test/addons-napi/test_string/test.js:74:15)
    at tryBlock (assert.js:656:5)
    at innerThrows (assert.js:675:18)
    at Function.throws (assert.js:702:3)
    at Object.<anonymous> (/home/ruben/repos/node/node/test/addons-napi/test_string/test.js:73:8)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)
Command: out/Release/node /home/ruben/repos/node/node/test/addons-napi/test_string/test.js
[02:57|% 100|+ 1970|-   1]: Done

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int. PR-URL: #15611 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int. PR-URL: nodejs#15611 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int. Backport-PR-URL: #19447 PR-URL: #15611 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@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 Sep 25, 2017

n-api: add check for large strings

6fdd2e4

n-api uses size_t for the size of strings when specifying string lengths. V8 only supports a size of int. Add a check so that an error will be returned if the user passes in a string with a size larger than will fit into an int.

mhdawson force-pushed the napi-string-check branch from 5a90899 to 6fdd2e4 Compare September 25, 2017 15:42

mhdawson mentioned this pull request Sep 25, 2017

n-api: fix warning about size_t compare with int #15508

Closed

4 tasks

addaleax approved these changes Sep 25, 2017

View reviewed changes

cjihrig approved these changes Sep 25, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

danbev approved these changes Sep 26, 2017

View reviewed changes

squash: address comments

d319160

squash: fix test failures on 32bit platforms

ff719da

mhdawson closed this Sep 29, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

mhdawson deleted the napi-string-check branch September 30, 2019 13:13

n-api: add check for large strings by mhdawson · Pull Request #15611 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: add check for large strings - #15611

n-api: add check for large strings#15611
mhdawson wants to merge 3 commits into
nodejs:masterfrom
mhdawson:napi-string-check

mhdawson commented Sep 25, 2017

Uh oh!

cjihrig left a comment

Uh oh!

mhdawson commented Sep 26, 2017

Uh oh!

addaleax commented Sep 26, 2017

Uh oh!

mhdawson commented Sep 26, 2017

Uh oh!

mhdawson commented Sep 26, 2017

Uh oh!

mhdawson commented Sep 28, 2017

Uh oh!

mhdawson commented Sep 28, 2017

Uh oh!

mhdawson commented Sep 28, 2017

Uh oh!

mhdawson commented Sep 29, 2017 •
edited
Loading

Uh oh!

Uh oh!

mhdawson commented Sep 29, 2017

Uh oh!

mhdawson commented Sep 29, 2017

Uh oh!

BridgeAR commented Oct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

n-api: add check for large strings by mhdawson · Pull Request #15611 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: add check for large strings - #15611

n-api: add check for large strings#15611mhdawson wants to merge 3 commits intonodejs:masternodejs/node:masterfrom mhdawson:napi-string-checkmhdawson/io.js:napi-string-checkCopy head branch name to clipboard

Conversation

mhdawson commented Sep 25, 2017

Checklist

Affected core subsystem(s)

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 26, 2017

Uh oh!

addaleax commented Sep 26, 2017

Uh oh!

mhdawson commented Sep 26, 2017

Uh oh!

mhdawson commented Sep 26, 2017

Uh oh!

mhdawson commented Sep 28, 2017

Uh oh!

mhdawson commented Sep 28, 2017

Uh oh!

mhdawson commented Sep 28, 2017

Uh oh!

mhdawson commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Sep 29, 2017

Uh oh!

mhdawson commented Sep 29, 2017

Uh oh!

BridgeAR commented Oct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

n-api: add check for large strings#15611
mhdawson wants to merge 3 commits into
nodejs:masterfrom
mhdawson:napi-string-check

mhdawson commented Sep 29, 2017 •
edited
Loading