Missing napi_type_tag_object for Napi::External? #1293

Activity

1. audetto commented on Feb 27, 2023

   

   audetto

   on Feb 27, 2023

   Author

   More actions

   I can still call it but I need

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

   which means the API works for things that are not just Objects.
2. KevinEady commented on Mar 10, 2023

   

   KevinEady

   on Mar 10, 2023

   Contributor

   More actions

   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.
3. KevinEady commented on Mar 10, 2023

   

   KevinEady

   on Mar 10, 2023

   Contributor

   More actions

   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"
4. audetto commented on Mar 11, 2023

   

   audetto

   on Mar 11, 2023

   Author

   More actions

   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.
5. gabrielschulhof commented on Mar 17, 2023

   

   gabrielschulhof

   on Mar 17, 2023

   Contributor

   More actions

   Opened nodejs/node#47141 for the core portion.
6. gabrielschulhof

   added 3 commits that reference this issue on Mar 21, 2023

   src: interject class TypeTaggable

   ...

   3149002

   src: interject class TypeTaggable

   ...

   016cac8

   src: interject class TypeTaggable

   ...

   f78e0c5
7. gabrielschulhof commented on Mar 21, 2023

   

   gabrielschulhof

   on Mar 21, 2023

   Contributor

   More actions

   @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.
8. audetto commented on Mar 21, 2023

   

   audetto

   on Mar 21, 2023

   Author

   More actions

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

   👍React with 👍1Reacted by Gabriel Schulhof
9. mhdawson

   added a commit that references this issue on Mar 27, 2023

   src: interject class TypeTaggable

   ...

   d013044
10. gabrielschulhof commented on Mar 28, 2023

    

    gabrielschulhof

    on Mar 28, 2023

    Contributor

    More actions

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

    closed this as completedon Mar 28, 2023
12. johnfrench3

    added a commit that references this issue on Aug 11, 2023

    src: interject class TypeTaggable

    ...

    a2e59bc