TLSWrap reports incorrect self_size

[paddybyers]

Heapdumped profiles on node >=4.2 show unfeasibly large retained sizes for native TLSWRAP instances.

I believe that this call: https://github.com/nodejs/node/blob/master/src/async-wrap.cc#L55 to self_size(), when on a TLSWrap, are in fact entering TLSWrap::Cast() and thus returning the this pointer value. I don't know yet whether the problem is in the way it is being called or in the declaration in TLSWrap but for some reason the self_size() call is resolving to the wrong vtable entry. Someone more expert will I am sure find the root cause quicker than I will.

/cc @indutny is this yours?

[indutny]

@paddybyers I'm not exactly sure what do you ask about. self_size() is usually very low for AsyncWrap instances, unless it is overridden by some other class. If it is big - it is very likely that it is properly overridden by TLSWrap, if it is not - it defaults to AsyncWrap::self_size and it is probably a bug.

[ChALkeR]

@paddybyers TLSWrap instances are quite big, I believe. What numbers are you observing, specifically?

[paddybyers]

I realise that what I'm saying isn't that believable but please bear with me.

Here's a simple test:

var heapdump = require('heapdump');
var tls = require('tls');

tls.connect(443, 'www.google.com', function(err) {
    if(err) {
        console.err('connect returned err: ', err);
        process.exit(1);
    }
    console.log('writing snapshot');
    heapdump.writeSnapshot();
    process.exit(0);
});

Here's the dump:

You can see that the value for Shallow Size for the sole TLSWrap is a pointer value (after casting to an int).

Now try running the above test with the changes here: https://github.com/paddybyers/node-1/tree/log_tlswrap_retained_info

I'm logging the constructor of RetainedAsyncInfo where it obtains the incorrect length_ value, and the TLSWrap::Cast() method where the call to self_size() is ending up. Here's the corresponding log from running the test above:

TLSWrap::Cast: cast() = 50333104
TLSWrap::Cast: cast() = 50333104
writing snapshot
RetainedAsyncInfo: label_ = TTYWRAP, wrap->self_size() = 496
RetainedAsyncInfo: label_ = SIGNALWRAP, wrap->self_size() = 224
RetainedAsyncInfo: label_ = TTYWRAP, wrap->self_size() = 496
TLSWrap::Cast: cast() = 50333104
TLSWrap::Cast: cast() = 50333104
RetainedAsyncInfo: label_ = TLSWRAP, wrap->self_size() = 50333104
RetainedAsyncInfo: label_ = TCPWRAP, wrap->self_size() = 416

You can see that the call to self_size() in the RetainedAsyncInfo constructor is in fact calling TLSWrap::Cast() and returning the this value. Something is wrong in the way that the TLSWrap is being unwrapped when passed to the RetainedAsyncInfo.

It is just a matter of good fortune that this is harmlessly returning an invalid value and sooner or later there will be a layout change and something will properly break.

I've seen this behaviour on Linux and OSX, on 4.2.2 and 5.1.0, with prebuilt packages and with node build from source. The above run was on OSX 10.10.5 with Xcode tools:

c++ --version
Apple LLVM version 7.0.0 (clang-700.1.76)
Target: x86_64-apple-darwin14.5.0
Thread model: posix

[paddybyers]

/cc @bnoordhuis any idea?

[indutny]

Blergh, I know what's happening. This async wrap code is just terribly broken.

@trevnorris RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) { assumes that it can Unwrap<AsyncWrap> while in reality it can't just blindly cast internal pointer to AsyncWrap*.

@paddybyers sorry, I didn't understand your description at first, but now everything is clear. Thank you!

[indutny]

@paddybyers may I ask you to give a try to a following patch?

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index bc830db..94f70eb 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -36,11 +36,11 @@ TLSWrap::TLSWrap(Environment* env,
                  Kind kind,
                  StreamBase* stream,
                  SecureContext* sc)
-    : SSLWrap<TLSWrap>(env, sc, kind),
-      StreamBase(env),
-      AsyncWrap(env,
+    : AsyncWrap(env,
                 env->tls_wrap_constructor_function()->NewInstance(),
                 AsyncWrap::PROVIDER_TLSWRAP),
+      SSLWrap<TLSWrap>(env, sc, kind),
+      StreamBase(env),
       sc_(sc),
       stream_(stream),
       enc_in_(nullptr),
diff --git a/src/tls_wrap.h b/src/tls_wrap.h
index 47cbf27..31d1952 100644
--- a/src/tls_wrap.h
+++ b/src/tls_wrap.h
@@ -21,9 +21,9 @@ namespace crypto {
   class SecureContext;
 }

-class TLSWrap : public crypto::SSLWrap<TLSWrap>,
-                public StreamBase,
-                public AsyncWrap {
+class TLSWrap : public AsyncWrap,
+                public crypto::SSLWrap<TLSWrap>,
+                public StreamBase {
  public:
   ~TLSWrap() override;

[paddybyers]

@indutny: yes, that works. In fact I had already tried that but I wasn't sure if it was a general enough change or if there needed to be a way to Unwrap with a dynamic cast.

[indutny]

@paddybyers technically speaking, I think async-wrap.cc could select class based on the class_id instead of casting everything to AsyncWrap, but at least this patch is something immediately available to us. Thanks for testing.

@trevnorris what are your thoughts on this?

[indutny]

Should be fixed by #4268

[wprater]

Im still seeing this strange behavior on 4.4.3. I cannot seem to locate which version of node this was landed on?

EDIT: this was on 4.2.4. I cannot replicate this on 4.4.3