Skip to content

Conversation

gabrielschulhof
Copy link
Contributor

When napi_create_string_* receives a null pointer as its second
argument, it must null-check it before passing it into V8, otherwise a
crash will occur.

Signed-off-by: @gabrielschulhof

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 4, 2021
@gabrielschulhof gabrielschulhof added node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jun 4, 2021
When `napi_create_string_*` receives a null pointer as its second
argument, it must null-check it before passing it into V8, otherwise a
crash will occur.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
@gabrielschulhof gabrielschulhof force-pushed the node-api-fix-string-null branch from e86316d to b5dc717 Compare June 4, 2021 06:38
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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

str should be allowed to be nullptr if length is 0.

@gabrielschulhof
Copy link
Contributor Author

@addaleax I added an allowance for NULL when the length given is zero. PTAL!

@nodejs-github-bot
Copy link
Collaborator

@@ -1483,6 +1483,8 @@ napi_status napi_create_string_latin1(napi_env env,
size_t length,
napi_value* result) {
CHECK_ENV(env);
if (length > 0)
CHECK_ARG(env, str);
Copy link
Member

Choose a reason for hiding this comment

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

The naming of these macros is really unfortunate. Everywhere else, CHECK_* generally implies an assert.

@mhdawson
Copy link
Member

@nodejs-github-bot
Copy link
Collaborator

gabrielschulhof added a commit that referenced this pull request Jun 11, 2021
When `napi_create_string_*` receives a null pointer as its second
argument, it must null-check it before passing it into V8, otherwise a
crash will occur.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #38923
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in d615aeb.

@gabrielschulhof gabrielschulhof deleted the node-api-fix-string-null branch June 11, 2021 16:04
danielleadams pushed a commit that referenced this pull request Jun 13, 2021
When `napi_create_string_*` receives a null pointer as its second
argument, it must null-check it before passing it into V8, otherwise a
crash will occur.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #38923
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
When `napi_create_string_*` receives a null pointer as its second
argument, it must null-check it before passing it into V8, otherwise a
crash will occur.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #38923
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@richardlau
Copy link
Member

This is blocked from landing on v14.x-staging as it depends on #37217.

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++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants