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: tighten null-checking and clean up last error #12539

Closed

Conversation

gabrielschulhof
Copy link
Contributor

We left out null-checks for many of the parameters passed to our APIs.
In particular, arguments of type napi_value were often accepted
without a null-check, even though they should never be null.

Additionally, many APIs simply returned napi_ok on success. This
leaves in place an error that may have occurred in a previous N-API
call. Others (those which perform NAPI_PREAMBLE(env) at the top)
OTOH explicitly clear the last error before proceeding. With this
modification all APIs explicitly clear the last error on success.

Fixes nodejs/abi-stable-node#227

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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++. node-api Issues and PRs related to the Node-API. labels Apr 20, 2017
src/node_api.cc Outdated
}

napi_status napi_set_last_error(napi_env env, napi_status error_code,
static napi_status napi_set_last_error(napi_env env, napi_status error_code,
Copy link
Contributor

Choose a reason for hiding this comment

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

This change throws off the indentation of the following lines.

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

@addaleax
Copy link
Member

This would need a rebase, and since #12366 most of the internals here are wrapped in an anonymous namespace so the statics here would be redundant

We left out null-checks for many of the parameters passed to our APIs.
In particular, arguments of type `napi_value` were often accepted
without a null-check, even though they should never be null.

Additionally, many APIs simply returned `napi_ok` on success. This
leaves in place an error that may have occurred in a previous N-API
call. Others (those which perform `NAPI_PREAMBLE(env)` at the top)
OTOH explicitly clear the last error before proceeding. With this
modification all APIs explicitly clear the last error on success.

Fixes nodejs/abi-stable-node#227
@gabrielschulhof
Copy link
Contributor Author

@addaleax rebased. I believe the static function touched in this PR are outside of a namespace, but you're right, there are now functions in src/node_api.cc which need no longer be static.

@gabrielschulhof
Copy link
Contributor Author

@cjihrig all good now?

@gabrielschulhof
Copy link
Contributor Author

@mhdawson @jasongin @addaleax thanks for taking a look!

@mhdawson
Copy link
Member

Ran out of time today, will plan to land tomorrow (Tuesday)

@mhdawson mhdawson self-assigned this Apr 24, 2017
@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

CI good landing.

@jasongin
Copy link
Member

@mhdawson Did you intend to merge this?
I'm just reminding you because I'm waiting on it for #12524

@addaleax
Copy link
Member

Landed in ba7bac5

@addaleax addaleax closed this Apr 25, 2017
addaleax pushed a commit that referenced this pull request Apr 25, 2017
We left out null-checks for many of the parameters passed to our APIs.
In particular, arguments of type `napi_value` were often accepted
without a null-check, even though they should never be null.

Additionally, many APIs simply returned `napi_ok` on success. This
leaves in place an error that may have occurred in a previous N-API
call. Others (those which perform `NAPI_PREAMBLE(env)` at the top)
OTOH explicitly clear the last error before proceeding. With this
modification all APIs explicitly clear the last error on success.

Fixes: nodejs/abi-stable-node#227
PR-URL: #12539
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mhdawson
Copy link
Member

Just got back after lunch and tried to push seeing the CI green, but see @addaleax already has.

@gabrielschulhof gabrielschulhof deleted the 227-add-null-checks branch April 26, 2017 19:24
@jasnell jasnell mentioned this pull request May 11, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
We left out null-checks for many of the parameters passed to our APIs.
In particular, arguments of type `napi_value` were often accepted
without a null-check, even though they should never be null.

Additionally, many APIs simply returned `napi_ok` on success. This
leaves in place an error that may have occurred in a previous N-API
call. Others (those which perform `NAPI_PREAMBLE(env)` at the top)
OTOH explicitly clear the last error before proceeding. With this
modification all APIs explicitly clear the last error on success.

Fixes: nodejs/abi-stable-node#227
PR-URL: nodejs#12539
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
We left out null-checks for many of the parameters passed to our APIs.
In particular, arguments of type `napi_value` were often accepted
without a null-check, even though they should never be null.

Additionally, many APIs simply returned `napi_ok` on success. This
leaves in place an error that may have occurred in a previous N-API
call. Others (those which perform `NAPI_PREAMBLE(env)` at the top)
OTOH explicitly clear the last error before proceeding. With this
modification all APIs explicitly clear the last error on success.

Fixes: nodejs/abi-stable-node#227
Backport-PR-URL: #19447
PR-URL: #12539
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
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.

napi_call_function (and maybe others) - we assume napi_value passed in is not NULL
6 participants