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.

Sign up for GitHub

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

Jump to bottom

Add error checking in String::New function #1015

Closed
wants to merge 3 commits into from
Closed

Add error checking in String::New function #1015

wants to merge 3 commits into from

Conversation

JckXia
Copy link
Member

@JckXia JckXia commented Jun 28, 2021

Added an if statement to String::New(env, const char * str) to make sure that str pointer is not null

@JckXia
Copy link
Member Author

JckXia commented Jun 28, 2021

Should I also add this if statement to the otherString::Newoverloads that accept a C char pointer? Also for the overload String::New(env, const char *, length), it doesn't seem like there are any checks in place to make sure that the length param is indeed the length of the string passed in. If I pass a value that exceeds the length of the string ( in this case String::New(env, "abcde",46)), I get this result:

stringLengthCheck
Is this expected behavior? Thank you!
To reproduce the above result, I defined a C++ function returning String::New(env, "abcde",46). From the JS side, I printed out the return value of this Cpp function using console.log()

KevinEady
KevinEady requested changes Jun 28, 2021
View reviewed changes
test/name.js Outdated
Comment on lines 10 to 15
try{
binding.name.nullStringShouldThrow();
}catch(e)
{
assert.strictEqual(e.message, 'String::New recieved a nullpointer as a value');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use assert.throws()

napi-inl.h Outdated
@@ -900,6 +900,11 @@ inline String String::New(napi_env env, const std::u16string& val) {
}

inline String String::New(napi_env env, const char* val) {
if (val == nullptr) {
NAPI_THROW(
TypeError::New(env, "String::New recieved a nullpointer as a value"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo 😉

@KevinEady KevinEady mentioned this pull request Jul 2, 2021
Closed
@KevinEady KevinEady self-requested a review July 9, 2021 13:04
KevinEady
KevinEady approved these changes Jul 9, 2021
View reviewed changes
Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mhdawson
mhdawson approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mhdawson pushed a commit that referenced this pull request Jul 9, 2021
@JckXia @mhdawson
test: dd check for nullptr inside String init
da50b51 
PR-URL: #1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Jul 9, 2021

Landed in da50b51

@mhdawson mhdawson closed this Jul 9, 2021
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021
@JckXia
test: dd check for nullptr inside String init
4f9b270 
PR-URL: nodejs#1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021
@JckXia
test: dd check for nullptr inside String init
016b827 
PR-URL: nodejs#1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
test: dd check for nullptr inside String init
0abece5 
PR-URL: nodejs/node-addon-api#1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
test: dd check for nullptr inside String init
d69a68f 
PR-URL: nodejs/node-addon-api#1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
test: dd check for nullptr inside String init
aebedd2 
PR-URL: nodejs/node-addon-api#1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
@JckXia JckXia deleted the add-check-for-null-str branch December 14, 2022 00:46
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
test: dd check for nullptr inside String init
23e9dfc 
PR-URL: nodejs/node-addon-api#1015
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@KevinEady KevinEady KevinEady approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JckXia @mhdawson @KevinEady