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

n-api: add check for large strings#15611

Closed
mhdawson wants to merge 3 commits into
nodejs:masterfrom
mhdawson:napi-string-check
Closed

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

Conversation

@mhdawson

@mhdawson mhdawson commented Sep 25, 2017

Copy link
Copy Markdown
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
@mhdawson
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 mhdawson mentioned this pull request Sep 25, 2017
Closed
4 tasks
cjihrig
cjihrig approved these changes Sep 25, 2017
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

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

mhdawson commented Sep 26, 2017

Copy link
Copy Markdown
Member Author

@addaleax

addaleax commented Sep 26, 2017

Copy link
Copy Markdown
Member

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

@mhdawson

mhdawson commented Sep 26, 2017

Copy link
Copy Markdown
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

mhdawson commented Sep 26, 2017

Copy link
Copy Markdown
Member Author

Pushed change to address comments.

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

@mhdawson

mhdawson commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

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

@mhdawson

mhdawson commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

Pushed change to fix test failure on 32 bit platforms.

@mhdawson

mhdawson commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

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

@mhdawson

mhdawson commented Sep 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

mhdawson commented Sep 29, 2017

Copy link
Copy Markdown
Member Author

Arm failures where:#15655

Windows failure was: #15558

CI looks good landing.

@mhdawson

mhdawson commented Sep 29, 2017

Copy link
Copy Markdown
Member Author

Landed as cec6e21

@mhdawson mhdawson closed this Sep 29, 2017
mhdawson added a commit that referenced this pull request Sep 29, 2017
@mhdawson
n-api: add check for large strings
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>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
@mhdawson @addaleax
n-api: add check for large strings
07168a9 
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>
@BridgeAR

BridgeAR commented Oct 2, 2017

Copy link
Copy Markdown
Member

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
@mhdawson @MylesBorins
n-api: add check for large strings
cbe5d7a 
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>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
@mhdawson @MylesBorins
n-api: add check for large strings
6582dcb 
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>
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@mhdawson @MylesBorins
n-api: add check for large strings
5f469a2 
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>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@mhdawson
n-api: add check for large strings
7e6c9f1 
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>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mhdawson @MylesBorins
n-api: add check for large strings
00d094f 
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>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
@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

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@danbev danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mhdawson @addaleax @BridgeAR @danbev @jasnell @cjihrig @nodejs-github-bot