src: fix multi-base class ObjectWrap::Unwrap<T>()

Fix pointer unwrapping when T is a class with more than one base class.

Before this commit, the wrapped void* pointer was cast directly to T*
without going through ObjectWrap* first, possibly leading to a class
instance pointer that points to the wrong vtable.

This change required some cleanup in various files; some classes
used private rather than public inheritance, others didn't derive
from ObjectWrap at all...

Fixes #6188.

5 files changed, 44 insertions, 29 deletions

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 132fa911de..da636556b1 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -212,8 +212,8 @@ class ContextifyContext {
       const PropertyCallbackInfo<Value>& args) {
     HandleScope scope(node_isolate);
 
-    Local<Object> data = args.Data()->ToObject();
-    ContextifyContext* ctx = ObjectWrap::Unwrap<ContextifyContext>(data);
+    ContextifyContext* ctx = NULL;
+    NODE_UNWRAP(args.Data().As<Object>(), ContextifyContext, ctx);
 
     Local<Object> sandbox = PersistentToLocal(node_isolate, ctx->sandbox_);
     Local<Value> rv = sandbox->GetRealNamedProperty(property);
@@ -236,8 +236,8 @@ class ContextifyContext {
       const PropertyCallbackInfo<Value>& args) {
     HandleScope scope(node_isolate);
 
-    Local<Object> data = args.Data()->ToObject();
-    ContextifyContext* ctx = ObjectWrap::Unwrap<ContextifyContext>(data);
+    ContextifyContext* ctx = NULL;
+    NODE_UNWRAP(args.Data().As<Object>(), ContextifyContext, ctx);
 
     PersistentToLocal(node_isolate, ctx->sandbox_)->Set(property, value);
   }
@@ -248,8 +248,8 @@ class ContextifyContext {
       const PropertyCallbackInfo<Integer>& args) {
     HandleScope scope(node_isolate);
 
-    Local<Object> data = args.Data()->ToObject();
-    ContextifyContext* ctx = ObjectWrap::Unwrap<ContextifyContext>(data);
+    ContextifyContext* ctx = NULL;
+    NODE_UNWRAP(args.Data().As<Object>(), ContextifyContext, ctx);
 
     Local<Object> sandbox = PersistentToLocal(node_isolate, ctx->sandbox_);
     Local<Object> proxy_global = PersistentToLocal(node_isolate,
@@ -269,8 +269,8 @@ class ContextifyContext {
       const PropertyCallbackInfo<Boolean>& args) {
     HandleScope scope(node_isolate);
 
-    Local<Object> data = args.Data()->ToObject();
-    ContextifyContext* ctx = ObjectWrap::Unwrap<ContextifyContext>(data);
+    ContextifyContext* ctx = NULL;
+    NODE_UNWRAP(args.Data().As<Object>(), ContextifyContext, ctx);
 
     bool success = PersistentToLocal(node_isolate,
                                      ctx->sandbox_)->Delete(property);
@@ -286,15 +286,15 @@ class ContextifyContext {
       const PropertyCallbackInfo<Array>& args) {
     HandleScope scope(node_isolate);
 
-    Local<Object> data = args.Data()->ToObject();
-    ContextifyContext* ctx = ObjectWrap::Unwrap<ContextifyContext>(data);
+    ContextifyContext* ctx = NULL;
+    NODE_UNWRAP(args.Data().As<Object>(), ContextifyContext, ctx);
 
     Local<Object> sandbox = PersistentToLocal(node_isolate, ctx->sandbox_);
     args.GetReturnValue().Set(sandbox->GetPropertyNames());
   }
 };
 
-class ContextifyScript : ObjectWrap {
+class ContextifyScript : public ObjectWrap {
  private:
   Persistent<Script> script_;
 
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 9a3cfe9390..e4b983b4c9 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -886,7 +886,8 @@ void SSLWrap<Base>::GetPeerCertificate(
     const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   Environment* env = w->env();
 
   Local<Object> info = Object::New();
@@ -1020,7 +1021,8 @@ template <class Base>
 void SSLWrap<Base>::GetSession(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
 
   SSL_SESSION* sess = SSL_get_session(w->ssl_);
   if (sess == NULL)
@@ -1041,7 +1043,8 @@ template <class Base>
 void SSLWrap<Base>::SetSession(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
 
   if (args.Length() < 1 ||
       (!args[0]->IsString() && !Buffer::HasInstance(args[0]))) {
@@ -1079,7 +1082,8 @@ template <class Base>
 void SSLWrap<Base>::LoadSession(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   Environment* env = w->env();
 
   if (args.Length() >= 1 && Buffer::HasInstance(args[0])) {
@@ -1111,7 +1115,8 @@ void SSLWrap<Base>::LoadSession(const FunctionCallbackInfo<Value>& args) {
 template <class Base>
 void SSLWrap<Base>::IsSessionReused(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   bool yes = SSL_session_reused(w->ssl_);
   args.GetReturnValue().Set(yes);
 }
@@ -1120,7 +1125,8 @@ void SSLWrap<Base>::IsSessionReused(const FunctionCallbackInfo<Value>& args) {
 template <class Base>
 void SSLWrap<Base>::ReceivedShutdown(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   bool yes = SSL_get_shutdown(w->ssl_) == SSL_RECEIVED_SHUTDOWN;
   args.GetReturnValue().Set(yes);
 }
@@ -1129,9 +1135,8 @@ void SSLWrap<Base>::ReceivedShutdown(const FunctionCallbackInfo<Value>& args) {
 template <class Base>
 void SSLWrap<Base>::EndParser(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
-
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
-
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   w->hello_parser_.End();
 }
 
@@ -1140,7 +1145,8 @@ template <class Base>
 void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
 
   ClearErrorOnReturn clear_error_on_return;
   (void) &clear_error_on_return;  // Silence unused variable warning.
@@ -1153,7 +1159,8 @@ void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& args) {
 template <class Base>
 void SSLWrap<Base>::IsInitFinished(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   bool yes = SSL_is_init_finished(w->ssl_);
   args.GetReturnValue().Set(yes);
 }
@@ -1164,7 +1171,8 @@ template <class Base>
 void SSLWrap<Base>::VerifyError(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
 
   // XXX(indutny) Do this check in JS land?
   X509* peer_cert = SSL_get_peer_certificate(w->ssl_);
@@ -1230,7 +1238,8 @@ template <class Base>
 void SSLWrap<Base>::GetCurrentCipher(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
   Environment* env = w->env();
 
   OPENSSL_CONST SSL_CIPHER* c = SSL_get_current_cipher(w->ssl_);
@@ -1325,7 +1334,8 @@ void SSLWrap<Base>::GetNegotiatedProto(
     const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
 
   if (w->is_client()) {
     if (w->selected_npn_proto_.IsEmpty() == false) {
@@ -1351,7 +1361,8 @@ template <class Base>
 void SSLWrap<Base>::SetNPNProtocols(const FunctionCallbackInfo<Value>& args) {
   HandleScope scope(node_isolate);
 
-  Base* w = ObjectWrap::Unwrap<Base>(args.This());
+  Base* w = NULL;
+  NODE_UNWRAP(args.This(), Base, w);
 
   if (args.Length() < 1 || !Buffer::HasInstance(args[0]))
     return ThrowTypeError("Must give a Buffer as first argument");
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 2ae8794031..c37285ba8d 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -57,7 +57,7 @@ extern X509_STORE* root_cert_store;
 // Forward declaration
 class Connection;
 
-class SecureContext : ObjectWrap {
+class SecureContext : public ObjectWrap {
  public:
   static void Initialize(Environment* env, v8::Handle<v8::Object> target);
 
diff --git a/src/node_object_wrap.h b/src/node_object_wrap.h
index 5c6ab2865b..683123559f 100644
--- a/src/node_object_wrap.h
+++ b/src/node_object_wrap.h
@@ -56,7 +56,11 @@ class NODE_EXTERN ObjectWrap {
   static inline T* Unwrap(v8::Handle<v8::Object> handle) {
     assert(!handle.IsEmpty());
     assert(handle->InternalFieldCount() > 0);
-    return static_cast<T*>(handle->GetAlignedPointerFromInternalField(0));
+    // Cast to ObjectWrap before casting to T.  A direct cast from void
+    // to T won't work right when T has more than one base class.
+    void* ptr = handle->GetAlignedPointerFromInternalField(0);
+    ObjectWrap* wrap = static_cast<ObjectWrap*>(ptr);
+    return static_cast<T*>(wrap);
   }
 
 
diff --git a/src/node_stat_watcher.h b/src/node_stat_watcher.h
index fd38c8b42f..9d5e1fd282 100644
--- a/src/node_stat_watcher.h
+++ b/src/node_stat_watcher.h
@@ -30,7 +30,7 @@
 
 namespace node {
 
-class StatWatcher : ObjectWrap {
+class StatWatcher : public ObjectWrap {
  public:
   static void Initialize(v8::Handle<v8::Object> target);
   inline Environment* env() const { return env_; }