Assertion failure (possible security hole?)

[cscott]

> var sqlite3 = require('sqlite3');
> db = new sqlite3.Database('foo.db')
> o = { close: db.close };
> o.close()
node: /home/cananian/.node-gyp/0.10.22/src/node_object_wrap.h:61: static T* node::ObjectWrap::Unwrap(v8::Handle<v8::Object>) [with T = node_sqlite3::Database]: Assertion `handle->InternalFieldCount() > 0' failed.
Aborted

You should use the Signature parameter to v8 FunctionTemplate to prevent calling methods with the improper receiver type. (And then you should also use accessor.Holder() instead of accessor.This() when you unwrap.)

[cscott]

More info:
https://groups.google.com/forum/#!topic/v8-users/Axf4hF_RfZo
https://www.mail-archive.com/v8-dev@googlegroups.com/msg13367.html
kkaefer/node-cpp-modules#8

[cscott]

FWIW, the assertion checks the case where you assign the method to an object with no internal fields. The security issue is if you manage to pass an object which does have an internal field (like from another wrapper object) but for a different type of object. Then you're going to go ahead and scribble on that other object and who knows what happens after that.

[springmeyer]

Thank you very much for this report @cscott. Would it be possible to provide a patch? If not I'll take a look at this when I find some time to study the issue more.

[springmeyer]

started work on this. I can confirm both the crash and the method of fixing it (turning it into an Illegal invocation error): master...fix-unsafe-access

@cscott - any sense if nodejs/node-v0.x-archive#6690 is actionable or not? I'd much prefer to have this blessed by node core or NAN before changing node-sqlite3 to not use node core macros.

/cc @kkoopa @rvagg

[cscott]

Upstream merged nodejs/node-v0.x-archive#6690 (in nodejs/node-v0.x-archive#7261). So on the node-sqlite3 front, you just need to be using args.Holder(). Of course, it will probably be a bit more complicated since you want to maintain compatibility with different node versions, etc.

[springmeyer]

Thanks @cscott - I had previously done master...fix-unsafe-access. Does that look right?

[daniellockyer]

AFAICT this is now an Illegal invocation error so I'm going to close this 🙂