-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
N-API: Type safety for napi_get_value_external/napi_unwrap #28164
Comments
is calling napi_get_value_external on something you don't own not a bug on its own? this looks like working as intended from my perspective. Additionally, having generic |
No, because once the external value passes through JavaScript land and comes back to my native module (which, as far as I can tell, is the whole point of externals), there is no guarantee of whether or not the external that gets passed back is one that my native module created. That's the whole point of adding type information to an external: to find out whether or not I own it, in a way that's guaranteed, not merely likely, to work.
Yes, but there's always some guarantee as to who owns which one. For example, there is a guarantee that, when one of my externals is finalized, the But there's no such guarantee for |
It's not that this feature couldn't be implemented but if you can't be sure the external that's passed to you is one you handed out, then you're arguably using it wrong. An idiomatic way of solving that is by storing the external as a value in a // index.js
const addon = require('./build/Release/addon.node');
const externals = new WeakMap();
module.exports = MyClass;
class MyClass {
constructor(...args) {
externals.set(this, addon.newExternal(...args));
}
act() {
const external = externals.get(this);
return external && addon.act(external);
}
} |
@bnoordhuis, that is not a solution. Reasons:
In short:
It is currently impossible to be sure. That's the whole point of my proposal. |
Just for some more context, the napi_external api more-or-less mirrors v8's External api, which looks like this: class V8_EXPORT External : public Value {
public:
static Local<External> New(Isolate* isolate, void* value);
V8_INLINE static External* Cast(Value* obj);
void* Value() const;
private:
static void CheckCast(v8::Value* obj);
}; This hasn't (as far as i am aware) ever been an issue for this api in v8, so i'm not yet convinced that this issue would suddenly appear for napi. We even use these v8 externals within node itself and they haven't been a source of unsafe behaviour. |
I think it’s good to first take a step back, and ask the question of what use cases there are for external that cannot be fulfilled by anything else, rather than jumping to the conclusion that N-API should provide such an interface. How are you using externals, may I ask? Is the external object like the native counterpart for a JavaScript object? In my experience, externals are a huge hammer that is best thought of as a tool of last resort. It is very unsafe, and also fairly memory-inefficient. (The underlying V8 |
@devsnek: Well, using externals is safe if only one code base (the application embedding V8) is able to create externals. In that case, you own all externals. But that's not the case with Node, which allows multiple native modules to be loaded at the same time. I can ensure that all of my externals are pointers to a data structure with a type identifier at the start, but that's it—I can't control what sort of data other native modules put in their externals. @TimothyGu: I notice that the V8 implementation of N-API won't allow an instance method created with Perhaps it would be better to attach type information to |
Okay, I think adding some sort of support for type-checking for However, this is not without technically challenges. The way |
If that isn't feasible, a somewhat cumbersome but hopefully simple solution would be along the lines of my original proposal for typed externals: a function that generates opaque unique type identifiers, and a variant of // An opaque identifier.
typedef struct napi_wrapper_type__* napi_wrapper_type;
// Creates a new napi_wrapper_type.
// Each call to this function yields a different napi_wrapper_type.
napi_status napi_create_wrapper_type(
napi_env env,
napi_wrapper_type* type
);
// Like napi_wrap, but takes a napi_wrapper_type parameter.
// Attaches it to the created napi_value.
napi_status napi_wrap_typed(
napi_env env,
napi_value js_object,
napi_wrapper_type type,
void* native_object,
napi_finalize finalize_cb,
void* finalize_hint,
napi_ref* result
);
// Like napi_unwrap, but takes a napi_wrapper_type parameter.
// Throws TypeError if the given object has a different (or no) napi_wrapper_type.
napi_status napi_unwrap_typed(napi_env env,
napi_value js_object,
napi_wrapper_type type,
void** result
); |
To be clear, that's not a design consideration for Node.js. It's not a sandbox for executing untrusted code. If the idea of a WeakMap or a JS shim is unpalatable to you (it's just idiomatic, it's not the only way to do things), then e.g.:
That could work but it would have to be investigated how inheritance ( |
Actually, If you try to pass an object other than an instance of the class to one of its prototype methods, you get
To illustrate, take a look at https://github.com/nodejs/node-addon-examples/blob/master/6_object_wrap/napi/addon.js#L6 and insert this line: obj.plusOne.apply({}); This will cause the above-mentioned exception. The reason for this scrutiny is here: Line 837 in 76bf7ee
That is, when we define a prototype method in N_API we add a Thus, if you wrap instances of a class declared with Additionally you can verify using So, basically, the type information you need is stored in V8 itself in the relationship between the class and its instances. For externals, there is no such guarantee. |
Thus, I would argue that the scope of this issue be restricted to |
@gabrielschulhof Did you see #28164 (comment)? The concern is that |
This is not safe against Object.setPrototypeOf, Object.create, etc. |
@TimothyGu the inability to access the Parameters other than Unless I misunderstood the use of const spoof = {};
Object.setPrototypeOf(spoof, addon.MyObject.prototype);
console.log(spoof instanceof addon.MyObject);
obj.plusOne.apply(spoof); and const spoof1 = Object.create(new addon.MyObject(11));
console.log(spoof1 instanceof addon.MyObject); // <-- added via an edit of the comment
console.log(spoof1.plusOne()); |
Both of the |
const spoof = new addon1.SomeNativeObject();
Object.setPrototypeOf(spoof, addon2.SomeOtherNativeObject.prototype);
addon2.someFunction(spoof); …will probably segfault, and |
@argv-minus-one yep, because |
I guess we don't currently safely support this kind of usage. |
Indeed. That's why I filed this issue. @bnoordhuis: This isn't only a sandbox issue. JS is also supposed to always be memory-safe, even in the face of shenanigans like I cannot store wrapped objects in |
Now that you all mention it, here's another alternative solution: make it so that
Not sure if the major JS engines are capable of restricting [[Prototype]] mutation like that, though. |
Prototype freezing is not something engines can currently do, although it has been discussed before (https://github.com/tc39/proposal-freeze-prototype) Also just to clarify the point about memory safety, at no point is js doing anything unsafe. The question is more about whether 1) you can guarantee the memory safety of your addon code (using stuff like ubsan i guess?) and 2) whether node guarantees the memory safety of napi apis. To this point, I think the api proposed in the OP makes sense. |
@argv-minus-one I don't think we need a // Apply a type tag to an object.
napi_status napi_type_tag_object(napi_env env,
napi_value obj,
void* type_ag);
// Check if an object is tagged as a certain type.
napi_status napi_check_object_type_tag(napi_env env,
napi_value obj,
void* type_tag,
bool* result); This can be achieved with current APIs too, but it's not that clean, because it "stains" the object with a property called // At the top of the file
static int DatabaseHandleTypeTagSource;
static int* DatabaseHandleTypeTag = &DatabaseHandleTypeTagSource;
// ---------------------------------
// To create a tagged object
napi_value js_db_handle, type_tag;
DatabaseHandle* db_handle = open_database(...);
napi_create_object(env, &js_db_handle);
napi_wrap(env, js_db_handle, db_handle, db_handle_finalize, NULL, NULL);
napi_create_external(env, DatabaseHandleTypeTag, NULL, NULL, &type_tag);
napi_property_descriptor prop = {
"typeTag", NULL, NULL, NULL, NULL, type_tag, napi_default, NULL
};
napi_define_properties(env, js_db_handle, 1, &prop);
// At this point, `js_db_handle` has all the necessary decorations to indicate
// at a later time that it is a wrapper for a pointer of type `DatabaseHandle`.
// ---------------------------------
// To validate and unwrap a tagged object argv[0]
napi_value js_type_tag;
napi_get_named_property(env, argv[0], "typeTag", &js_type_tag);
napi_valuetype type_tag_type;
napi_typeof(env, js_type_tag, &type_tag_type);
if (type_tag_type != napi_external) {
// Throw type error
return;
}
void* type_tag;
napi_get_value_external(env, js_type_tag, &type_tag);
if (type_tag != (void*)DatabaseHandleTypeTag) {
// Throw type error
return;
}
DatabaseHandle* db_handle;
napi_unwrap(env, argv[0], &db_handle);
// db_handle is now certainly of type `DatabaseHandle`. The N-API implementation of this would perform the exact same steps, but it would use a |
Looks good to me. If you do it that way, then the N-API docs need to make it clear that the type tag must be a pointer to memory that the calling module owns, such as a pointer to one of its own functions. Note that doing it with a regular property instead of By the way, you don't need two global variables to create a unique type tag, just one: static const void* DatabaseHandleTypeTag = &DatabaseHandleTypeTag; |
@argv-minus-one yeah, I guess even a symbol-keyed property where the symbol is stored in the addon can be found. |
`napi_instanceof()` is insufficient for reliably establishing the data type to which a pointer stored with `napi_wrap()` or `napi_create_external()` inside a JavaScript object points. Thus, we need a way to "mark" an object with a value that, when later retrieved, can unambiguously tell us whether it is safe to cast the pointer stored inside it to a certain structure. Such a check must survive loading/unloading/multiple instances of an addon, so we use UUIDs chosen *a priori*. Fixes: nodejs#28164
`napi_instanceof()` is insufficient for reliably establishing the data type to which a pointer stored with `napi_wrap()` or `napi_create_external()` inside a JavaScript object points. Thus, we need a way to "mark" an object with a value that, when later retrieved, can unambiguously tell us whether it is safe to cast the pointer stored inside it to a certain structure. Such a check must survive loading/unloading/multiple instances of an addon, so we use UUIDs chosen *a priori*. Fixes: #28164 Co-authored-by: Anna Henningsen <[email protected]> PR-URL: #28237 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Signed-off-by: Gabriel Schulhof <[email protected]>
`napi_instanceof()` is insufficient for reliably establishing the data type to which a pointer stored with `napi_wrap()` or `napi_create_external()` inside a JavaScript object points. Thus, we need a way to "mark" an object with a value that, when later retrieved, can unambiguously tell us whether it is safe to cast the pointer stored inside it to a certain structure. Such a check must survive loading/unloading/multiple instances of an addon, so we use UUIDs chosen *a priori*. Fixes: #28164 Co-authored-by: Anna Henningsen <[email protected]> PR-URL: #28237 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Signed-off-by: Gabriel Schulhof <[email protected]>
Is your feature request related to a problem? Please describe.
napi_get_value_external
is currently type-unsafe. It yields 32 or 64 arbitrary bits, with no guarantee as to whether they mean what the caller thinks they mean, nor which native module created the external. Even assuming that it's a pointer is unsafe and may result in segfault.Describe the solution you'd like
Please add a way to attach type information to values created by
napi_create_external
, and a way to check that type information when callingnapi_get_value_external
. The “type information” should be some sort of unique identifier generated by Node.js with an opaque C type, so that no two native modules can ever accidentally use the same type identifier for different types of external values.Suggested API:
Describe alternatives you've considered
Currently, I'm just assuming that
napi_get_value_external
gives me a pointer to something that's at leastsizeof(void *)
bytes long, and put a magic number at the beginning of the external data structure to identify its type. To make the magic number distinctive, it is a pointer to some data inside my module:As I've said above, this will result in undefined behavior (probably segfault) if some other native module calls
napi_create_external
with a value that isn't a valid pointer, and that external value somehow gets fed to my native module. Nor is it actually guaranteed that my magic number won't happen to be at the beginning of some other module's unrelated data structure, though it is highly unlikely.The text was updated successfully, but these errors were encountered: