Safety mistake? when calling binding function

[ozgrakkurt]

Shouldn't this be marked unsafe? https://github.com/infinyon/node-bindgen/blob/952a08a07b4438a1f1a71b4d0c4937c7d9b61dd2/nj-core/src/basic.rs#L328

If it call this with a null pointer or something like that, it is ub?

[digikata]

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.

[sehz]

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

[ozgrakkurt]

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

[sehz]

I guess we can mark as unsafe

[digikata]

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?

[ozgrakkurt]

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: https://github.com/infinyon/node-bindgen/blob/952a08a07b4438a1f1a71b4d0c4937c7d9b61dd2/nj-core/src/basic.rs#L377

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