Skip to content

Conversation

legendecas
Copy link
Member

As documented in napi_wrap section, the returned reference must be
deleted with napi_delete_reference in response to the finalize
callback, in which napi_unwrap and napi_remove_wrap is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with napi_unwrap
and napi_remove_wrap too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

This fix is intended to fix the use-after-free problem in #44141.

As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.
@nodejs-github-bot nodejs-github-bot added 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. labels Nov 10, 2022
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

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2022
@nodejs-github-bot nodejs-github-bot merged commit 926c830 into nodejs:main Nov 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 926c830

@legendecas legendecas deleted the node-api/napi-wrap-delete-ref branch November 18, 2022 09:32
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

PR-URL: #45406
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

PR-URL: nodejs#45406
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

PR-URL: #45406
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

PR-URL: #45406
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

PR-URL: #45406
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
As documented in napi_wrap section, the returned reference must be
deleted with `napi_delete_reference` in response to the finalize
callback, in which `napi_unwrap` and `napi_remove_wrap` is not
available.

When the reference needs to be deleted early, it should be
deleted after the wrapped value is not accessed with `napi_unwrap`
and `napi_remove_wrap` too.

This test is previously added in response to duplicating the test
https://github.com/nodejs/node-addon-api/blob/main/test/objectwrap_constructor_exception.cc
in the node-addon-api. As Napi::ObjectWrap<> is a subclass of
Napi::Reference<>, napi_remove_wrap in the destructor of
Napi::ObjectWrap<> is called before napi_delete_reference in the
destructor of Napi::Reference<>.

PR-URL: #45406
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants