Safety mistake? when calling binding function #232

Activity

1. digikata commented on Sep 5, 2023

   

   digikata

   on Sep 5, 2023

   Collaborator

   More actions

   It looks like create_reference is intended to set the pointer, so as long as the allocated pointer size is consistent and create_reference isn't trying to deref that pointer it seems ok. But thats and assumption, the code to create_reference should be checked.

   👍React with 👍1Reacted by ozgrakkurt
2. sehz commented on Sep 5, 2023

   

   sehz

   on Sep 5, 2023

   Last edited by sehz

   More actions

   macro napi_call_result already encapsulates unsafe thus caller can avoid it. See https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html

   👍React with 👍1Reacted by ozgrakkurt
3. ozgrakkurt commented on Sep 5, 2023

   

   ozgrakkurt

   on Sep 5, 2023

   ContributorAuthor

   More actions

   But the cons arg is a raw pointer so theoretically some user can pass anything in there and then it can be safety issue because there is no check about validity of that pointer. Also checked napi docs but it doesn't say anything
4. sehz commented on Sep 5, 2023

   

   sehz

   on Sep 5, 2023

   More actions

   I guess we can mark as unsafe

   👍React with 👍1Reacted by ozgrakkurt
5. digikata commented on Sep 5, 2023

   

   digikata

   on Sep 5, 2023

   Collaborator

   More actions

   It seems like there is a implicit conversion from ptr -> napi_ref that the lib should maybe make more explicit so any ptr can't be converted. If the creation of napi_ref was more protected, then it might be more secure to use the napi_ref. Is create_reference the only way to end up with a napi_ref?
6. ozgrakkurt commented on Sep 6, 2023

   

   ozgrakkurt

   on Sep 6, 2023

   ContributorAuthor

   More actions

   It seems like there is a implicit conversion from ptr -> napi_ref that the lib should maybe make more explicit so any ptr can't be converted. If the creation of napi_ref was more protected, then it might be more secure to use the napi_ref. Is create_reference the only way to end up with a napi_ref?

   I am looking into how to make it safe, also meanwhile working on temporary fix to segfault issue.

   This kind of safety issue is everywhere in the library so seems like we would need to do big refactor to fix it.

   For example this also seems unsafe to me:

   node-bindgen/nj-core/src/basic.rs

   Line 377 in 952a08a

   pub fn unwrap<T>(&self, js_this: napi_value) -> Result<&'static T, NjError> {

   not sure who has the ownership of the T value after this call