Skip to content

Conversation

@mhdawson
Copy link
Member

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

@nodejs-github-bot 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 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.
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, but I'd personally like a better error than "Unknown failure"

@mhdawson
Copy link
Member Author

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

@addaleax
Copy link
Member

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

@mhdawson
Copy link
Member Author

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.

@mhdawson
Copy link
Member Author

Pushed change to address comments.

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

@mhdawson
Copy link
Member Author

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

@mhdawson
Copy link
Member Author

Pushed change to fix test failure on 32 bit platforms.

@mhdawson
Copy link
Member Author

@mhdawson
Copy link
Member Author

mhdawson commented Sep 29, 2017

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/

@mhdawson
Copy link
Member Author

Arm failures where:#15655

Windows failure was: #15558

CI looks good landing.

@mhdawson
Copy link
Member Author

Landed as cec6e21

@mhdawson mhdawson closed this Sep 29, 2017
mhdawson added a commit that referenced this pull request Sep 29, 2017
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

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

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
@mhdawson mhdawson deleted the napi-string-check branch September 30, 2019 13:13
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++. node-api Issues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants