Node: N-API: Type safety for napi_get_value_external/napi_unwrap

[argv-minus-one]

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 calling napi_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:

// An opaque identifier.
typedef struct napi_external_type__* napi_external_type;

// Creates a new napi_external_type.
// Each call to this function yields a different napi_external_type.
napi_status napi_create_external_type(
    napi_env env,
    napi_external_type* type
);

// Like napi_create_external, but takes a napi_external_type parameter.
// Attaches it to the created napi_value.
napi_status napi_create_typed_external(
    napi_env env,
    void* data,
    napi_finalize finalize_cb,
    void* finalize_hint,
    napi_external_type type,
    napi_value* result
);

// Like napi_get_value_external, but takes a napi_external_type parameter.
// Throws TypeError if the given napi_value has a different (or no) napi_external_type.
napi_status napi_get_value_typed_external(
    napi_env env,
    napi_value value,
    napi_external_type type,
    void** result
);

Describe alternatives you've considered
Currently, I'm just assuming that napi_get_value_external gives me a pointer to something that's at least sizeof(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:

static const void *MAGIC = &MAGIC;

typedef struct {
    void *magic; // check if magic == MAGIC before use
    …
} my_data;

…

my_data *d;
napi_get_value_external(…, …, &d);
if (d->magic != MAGIC) {
    // bail
}

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.

[devsnek]

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 void* for random user data is a super super common pattern across the c and c++ ecosystems.

[argv-minus-one]

is calling napi_get_value_external on something you don't own not a bug on its own?

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. napi_unwrap has the same problem, by the way.

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.

Additionally, having generic void* for random user data is a super super common pattern across the c and c++ ecosystems.

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 void* passed to my finalizer function is very definitely the value of one of my own externals. No other native module can even find my finalizer (it's declared static, so it doesn't appear in my module's dylib/so/dll symbol table, and there are no pointers to it aside from those passed to N-API), let alone call it with some other value.

But there's no such guarantee for napi_get_value_external and napi_unwrap.

[bnoordhuis]

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 WeakMap, where the key is the object you hand out to your downstream user:

// 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);
  }
}

[argv-minus-one]

@bnoordhuis, that is not a solution. Reasons:

1. Using WeakMap for this would be a waste of CPU time and memory.
2. Managing the WeakMap in a JS wrapper module is unsafe, because other JS modules can require a native add-on directly.
3. Even if I construct a WeakMap using N-API calls, there is no guarantee that global.WeakMap is the genuine built-in WeakMap constructor, and hasn't been replaced by some other JS code.
4. I can't use a native hash table like std::unordered_map, either, because N-API does not expose any (almost-)unique identifier for objects that's guaranteed not to change throughout an object's lifetime (along the lines of Java's System.identityHashCode()).

In short:

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.

It is currently impossible to be sure. That's the whole point of my proposal.

[devsnek]

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.

[TimothyGu]

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 External object is a full-on object, even though all it has is a pointer.) We had been able to get rid of most instances of externals in our code base in favor of something safer, like the equivalent of napi_wrap().

[argv-minus-one]

@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: napi_wrap has the same problem. The documentation makes no guarantees, and napi_[un]wrap does not accept any sort of unique type identifier. napi_unwrap will happily unwrap any wrapper object, not just my wrapper objects.

I notice that the V8 implementation of N-API won't allow an instance method created with napi_define_class to be called on the wrong receiver object, so napi_unwrap of the thisArg seems safe, but that's it—there's no way to check whether a wrapper object passed as a parameter to a native function is of the correct type.

Perhaps it would be better to attach type information to napi_wrapped objects instead of externals? I'm fine with that too.

[TimothyGu]

Okay, I think adding some sort of support for type-checking for napi_wrapped objects is more workable. In particular, I think something like V8's FunctionTemplate::HasInstance() would be a worthy addition.

However, this is not without technically challenges. The way napi_define_class() and V8 APIs are defined, you don't really get access to the underlying FunctionTemplate from the defined class directly. How to essentially virtualize FunctionTemplate in an engine-independent way could be a challenge.

[argv-minus-one]

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 napi_[un]wrap that accepts/checks those type identifiers. Since napi_wrap already somehow associates an arbitrary void * with an arbitrary JS object, I assume it can associate a numeric type identifier too? API suggestion:

// 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
);

[bnoordhuis]

Managing the WeakMap in a JS wrapper module is unsafe, because other JS modules can require a native add-on directly.

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.:

1. Store the pointers you wrap in an external in a std::set or whatever, and
2. Check the pointer is part of the set when you unwrap an external later on.

Perhaps it would be better to attach type information to napi_wrapped objects instead of externals?

That could work but it would have to be investigated how inheritance (class Explode extends YourClass {}) affects storage/retrieval of the private data. I know the answer for V8 but I'm unsure about e.g. Spidermonkey.

[gabrielschulhof]

Actually, napi_wrap() is entirely type-safe if used in conjunction with napi_define_class(), because V8 ensures that prototype functions cannot be detached from the prototype and called on objects other than instances of the class.

If you try to pass an object other than an instance of the class to one of its prototype methods, you get

TypeError: Illegal invocation
    at Object.<anonymous> (/home/nix/node/node-addon-examples/6_object_wrap/napi/addon.js:8:26)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:837:10)
    at internal/main/run_main_module.js:17:11

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: https://github.com/nodejs/node/blob/76bf7ee77e7f5926af0729c5465bd98d8830f0e5/src/js_native_api_v8.cc#L837

That is, when we define a prototype method in N_API we add a v8::Signature() to its definition to ensure that the method can only be called when instances of the class or its subclasses are passed as the this parameter.

Thus, if you wrap instances of a class declared with napi_define_class() and receive the instances exclusively as this arguments to prototype methods, V8 will ensure that you never receive an instance containing the wrong kind of pointer.

Additionally you can verify using napi_instanceof() that any given JavaScript object is an instance of the constructor before you do a napi_unwrap() and cast the pointer.

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.

[gabrielschulhof]

Thus, I would argue that the scope of this issue be restricted to napi_get_value_external(), because with napi_unwrap() there is a way to ensure type safety.

[TimothyGu]

@gabrielschulhof Did you see https://github.com/nodejs/node/issues/28164#issuecomment-501507006? The concern is that napi_unwrap() does not guarantee type safety for parameters that are not necessarily this.

[TimothyGu]

Additionally you can verify using napi_instanceof() that any given JavaScript object is an instance of the constructor

This is not safe against Object.setPrototypeOf, Object.create, etc.

[gabrielschulhof]

@TimothyGu the inability to access the FunctionTemplate after the fact is indeed problematic. It also prevents doing inheritance via N-API. For example, we could never implement napi_status napi_inherit(napi_env env, napi_value parent_constructor, napi_value child_constructor); without it.

Parameters other than this can themselves be instances of classes whose constructors can be passed into napi_instanceof() for checking.

Unless I misunderstood the use of Object.setPrototypeOf() and/or Object.create(), both of the below sequences also throw the Illegal invocation exception:

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());

[gabrielschulhof]

Both of the instanceofs return true, but AFAICT the native side catches the shenanigans.

[argv-minus-one]

@gabrielschulhof:

const spoof = new addon1.SomeNativeObject();
Object.setPrototypeOf(spoof, addon2.SomeOtherNativeObject.prototype);
addon2.someFunction(spoof);

…will probably segfault, and napi_instanceof will not catch it.

[gabrielschulhof]

@argv-minus-one yep, because addon2.someFunction() is not a prototype method.

[gabrielschulhof]

I guess we don't currently safely support this kind of usage.

[argv-minus-one]

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 Object.setPrototypeOf or mutating global.WeakMap. I should be able to write a native add-on that doesn't compromise that memory-safety and never segfaults, but N-API as it stands makes that impossible.

I cannot store wrapped objects in std::set because N-API doesn't (and probably can't) expose an identity hash function; see #28195.

[argv-minus-one]

Now that you all mention it, here's another alternative solution: make it so that instanceof is guaranteed to return the correct result when applied to a class defined by napi_define_class. Specifically:

• Prohibit mutation of the [[Prototype]] (using Object.setPrototypeOf, __proto__, etc) of an object that has been napi_wrapped [edit: and its entire prototype chain].
• Freeze the prototype and [Symbol.hasInstance] properties of classes created by napi_define_class. (If an add-on uses napi_wrap without napi_define_class, it can do this explicitly.)
• Explain the above in the N-API docs for napi_unwrap, and that napi_instanceof can therefore be safely used to check the type of a napi_wrapped object.

Not sure if the major JS engines are capable of restricting [[Prototype]] mutation like that, though.

[devsnek]

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.

[gabrielschulhof]

@argv-minus-one I don't think we need a napi_external_type. Any pointer to a static value can serve as a type tag. So, we can have an API as simple as

// 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 "typeTag":

// 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 v8::Private instead of a simple string-keyed property.

[argv-minus-one]

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 v8::Private is unsafe, because JS code can copy the type tag to other objects. The type tag must be invisible to JS code.

By the way, you don't need two global variables to create a unique type tag, just one:

static const void* DatabaseHandleTypeTag = &DatabaseHandleTypeTag;

[gabrielschulhof]

@argv-minus-one yeah, I guess even a symbol-keyed property where the symbol is stored in the addon can be found.