node aborts when logging `process.binding('crypto').Connection.prototype`

[misterdjules]

As the subject says, node aborts when logging process.binding('crypto').Connection.prototype. It works fine with node 0.11.12:

➜  node git:(v0.11.12-release) ✗ ./node
> process.binding('crypto').Connection.prototype
{ encIn: [Function],
  clearOut: [Function],
  clearIn: [Function],
  encOut: [Function],
  clearPending: [Function],
  encPending: [Function],
  start: [Function],
  close: [Function],
  getPeerCertificate: [Function],
  getSession: [Function],
  setSession: [Function],
  loadSession: [Function],
  isSessionReused: [Function],
  isInitFinished: [Function],
  verifyError: [Function],
  getCurrentCipher: [Function],
  endParser: [Function],
  renegotiate: [Function],
  shutdown: [Function],
  getTLSTicket: [Function],
  newSessionDone: [Function],
  setMaxSendFragment: [Function],
  getNegotiatedProtocol: [Function],
  setNPNProtocols: [Function],
  getServername: [Function],
  setSNICallback: [Function] }
> %

But it aborts with node 0.11.14:

➜  node git:(v0.11.14-release) ✗ ./node
> process.binding('crypto').Connection.prototype
Assertion failed: (object->InternalFieldCount() > 0), function Unwrap, file ../src/util-inl.h, line 117.
[1]    66648 abort      ./node
➜  node git:(v0.11.14-release) ✗

Note that it's logging that aborts, just accessing the property does not make node abort:

➜  node git:(v0.11.14-release) ✗ ./node -e "process.binding('crypto').Connection.prototype"
➜  node git:(v0.11.14-release) ✗

The change that introduced this regression is somewhere between the 0.11.12 release and the 0.11.14 release, I haven't had the time to investigate more.

[trevnorris]

The addition of an internal field to the Connection object is my doing. There must be a getter on the prototype that is Unwrap'ing. Same happens with several others. e.g.:

> process.binding('crypto').SecureContext.prototype
node: ../src/util-inl.h:117: TypeName *node::Unwrap(v8::Local<v8::Object>) [TypeName = node::crypto::SecureContext]: Assertion `object->InternalFieldCount() > 0' failed.
Aborted (core dumped)

Though this works fine:

> Object.keys(process.binding('crypto').SecureContext.prototype)
[ '_external',
  'init',
  'setKey',
  'setCert',
  'addCACert',
  'addCRL',
  'addRootCerts',
  'setCiphers',
  'setECDHCurve',
  'setDHParam',
  'setOptions',
  'setSessionIdContext',
  'setSessionTimeout',
  'close',
  'loadPKCS12',
  'getTicketKeys',
  'setTicketKeys',
  'getCertificate',
  'getIssuer' ]

Will think on it.

[trevnorris]

Sure enough. From src/node_crypto.cc:

  NODE_SET_EXTERNAL(
      t->PrototypeTemplate(),
      "_external",
      CtxGetter);

Which is defined in src/node_internal.h:

inline void NODE_SET_EXTERNAL(v8::Handle<v8::ObjectTemplate> target,
                              const char* key,
                              v8::AccessorGetterCallback getter) {
  v8::Isolate* isolate = v8::Isolate::GetCurrent();
  v8::HandleScope handle_scope(isolate);
  v8::Local<v8::String> prop = v8::String::NewFromUtf8(isolate, key);
  target->SetAccessor(prop,
                      getter,
                      NULL,
                      v8::Handle<v8::Value>(),
                      v8::DEFAULT,
                      static_cast<v8::PropertyAttribute>(v8::ReadOnly |
                                                         v8::DontDelete));
}

This is going to be a problem with any getters on any prototypes that use Unwrap. Any ideas about how to deal with this situation?

[shaunc]

+1 -- getting this when using node-postgres clients in debugger.

[DenisGorbachev]

+1 getting it as well on v0.12.7

[jjongsma]

Seeing this crash in 0.12.7 whenever I set a connect timeout on "requests" module.

[trevnorris]

Not sure the best way to deal with the issue. Possibly just return early in case the object hasn't been setup yet.

[DenisGorbachev]

@trevnorris Sooo how do we get rid of this bug? :) It has plagued our production since release. Maybe there's some workaround we can apply while waiting for a fix?

[trevnorris]

Simplest solution would be to return undefined in these cases. Though I'm cautious to do this because of unforeseen side affects. And since a patch like that would have to migrate to latest node.

@bnoordhuis thoughts?

[DenisGorbachev]

@trevnorris I'm totally open to migrating to latest node :)

[trevnorris]

@DenisGorbachev Glad to hear it. :-)

I do think landing a fix in latest, and possibly v4, is correct. But also think back porting this to v0.12 would be a good idea too. Just looking for feedback from other core maintainers on possible alternative solutions.

[jasnell]

+1... let's land a fix in master and cherry pick it back to v4.x and v.12.x

[DenisGorbachev]

Omg, it's happening! This is the first bug I've reported for a major project, and it's getting fixed. Thank you @trevnorris @jasnell :)

[bnoordhuis]

@trevnorris Are you proposing to make Unwrap() stop asserting entirely or just for accessors?