Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: add check for large strings #15611

Closed
wants to merge 3 commits into from

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