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

Missing napi_type_tag_object for Napi::External? #1293

Closed
audetto opened this issue Feb 27, 2023 · 8 comments
Closed

Missing napi_type_tag_object for Napi::External? #1293

audetto opened this issue Feb 27, 2023 · 8 comments

Comments

@audetto
Copy link

audetto commented Feb 27, 2023

#1261

This PR added support for napi_type_tag_object, but it did so for Napi::Object.
This API is mostly useful for Napi::Externals which do no inherit from Napi::Object.

node-addon-api/napi.h

Lines 984 to 991 in b16c762

void TypeTag(const napi_type_tag* type_tag) const;
bool CheckTypeTag(const napi_type_tag* type_tag) const;
#endif // NAPI_VERSION >= 8
};
template <typename T>
class External : public Value {
public:

Should this api be moved down to Napi::Value?

@audetto
Copy link
Author

audetto commented Feb 27, 2023

I can still call it but I need

external.As<Object>().CheckTypeTag()

which means the API works for things that are not just Objects.

@KevinEady
Copy link
Contributor

We discussed this in the 10 March Node.js Node-API meeting. The documentation for the underlying napi_type_tag_object call says:

[in] js_object: The JavaScript object to be marked.

This implies that the API is limited to objects and not externals. The team needs to clarify the semantics of this function because it may have implications on other engines that implement Node-API.

@KevinEady
Copy link
Contributor

Path forward:

  1. Create a unit test in core that verifies that napi_type_tag_object and napi_check_type_tag can be used on externals. We expect this to work since a workaround was pointed out on Missing napi_type_tag_object for Napi::External? #1293 (comment)
  2. If so, update the Node-API documentation for those two methods to say "The JavaScript object or external"

@audetto
Copy link
Author

audetto commented Mar 11, 2023

Although I can see a usage of tagging for any javascript value (a bit like using the topmost bits of a c pointer), for externals it is a question of life of segfault.

I think the underlying api should be way more strict

  1. only create an external with a tag
  2. only extract the void * if the tag matches

otherwise, how do I differentiate an external created by my library and one created elsewhere? and how do they do the same? probably a big security hole.

@gabrielschulhof
Copy link
Contributor

Opened nodejs/node#47141 for the core portion.

gabrielschulhof added a commit to gabrielschulhof/node-addon-api that referenced this issue Mar 21, 2023
Derive class `TypeTaggable` from `Value`, and let it serve as the base
class for `Object` and `External`. That way, the type tagging is
implemented for both classes.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Refs: nodejs#1293
gabrielschulhof added a commit to gabrielschulhof/node-addon-api that referenced this issue Mar 21, 2023
Derive class `TypeTaggable` from `Value`, and let it serve as the base
class for `Object` and `External`. That way, the type tagging is
implemented for both classes.

Additionally, exclude deleted .js files from linting.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Refs: nodejs#1293
gabrielschulhof added a commit to gabrielschulhof/node-addon-api that referenced this issue Mar 21, 2023
Derive class `TypeTaggable` from `Value`, and let it serve as the base
class for `Object` and `External`. That way, the type tagging is
implemented for both classes.

Additionally, exclude deleted .js files from linting.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Refs: nodejs#1293
@gabrielschulhof
Copy link
Contributor

@audetto type-tagging externals and wrapped objects is certainly a good practice. The problem is that type tags came along later than the wrapped objects and externals themselves. Thus, we can no longer go back and restrict their usage, because that would break all the code out there that already uses them, potentially unrestricted. We can certainly advocate for the usage of type tags so add-on authors eventually adopt the practice of type-tagging the pointers they expose to JS.

@audetto
Copy link
Author

audetto commented Mar 21, 2023

I full understand the constraints.
I think as well the lack of tags must be a source of many vulnerabilities.

mhdawson pushed a commit that referenced this issue Mar 27, 2023
Derive class `TypeTaggable` from `Value`, and let it serve as the base
class for `Object` and `External`. That way, the type tagging is
implemented for both classes.

Additionally, exclude deleted .js files from linting.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Refs: #1293

PR-URL: #1298
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@gabrielschulhof
Copy link
Contributor

Fixed this in #1298. If you think more work needs to be done, please reopen this issue!

austinli64 added a commit to austinli64/node-addon-api that referenced this issue May 9, 2023
Derive class `TypeTaggable` from `Value`, and let it serve as the base
class for `Object` and `External`. That way, the type tagging is
implemented for both classes.

Additionally, exclude deleted .js files from linting.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Refs: nodejs/node-addon-api#1293

PR-URL: nodejs/node-addon-api#1298
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
Derive class `TypeTaggable` from `Value`, and let it serve as the base
class for `Object` and `External`. That way, the type tagging is
implemented for both classes.

Additionally, exclude deleted .js files from linting.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
Refs: nodejs/node-addon-api#1293

PR-URL: nodejs/node-addon-api#1298
Reviewed-By: Michael Dawson <midawson@redhat.com
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants