async_wrap: fix use-after-free for inspector session

This fixes the following condition:

    $ python -u tools/run-valgrind.py ./node_g test/sequential/test-inspector-async-call-stack.js
    [...]
    ==10848== Invalid read of size 4
    ==10848==    at 0x12F509E: node::AsyncWrap::provider_type() const (async_wrap-inl.h:34)
    ==10848==    by 0x12E7642: node::AsyncWrap::EmitTraceEventAfter() (async_wrap.cc:208)
    ==10848==    by 0x12F301B: node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) (async_wrap.cc:724)
    ==10848==    by 0x14516C6: node::inspector::(anonymous namespace)::JSBindingsConnection::OnMessage(v8::Local<v8::Value>) (inspector_js_api.cc:88)
    ==10848==    by 0x14514F1: node::inspector::(anonymous namespace)::JSBindingsConnection::JSBindingsSessionDelegate::SendMessageToFrontend(v8_inspector::StringView const&) (inspector_js_api.cc:57)
    ==10848==    by 0x14436AD: node::inspector::(anonymous namespace)::ChannelImpl::sendMessageToFrontend(v8_inspector::StringView const&) (inspector_agent.cc:232)
    ==10848==    by 0x1443627: node::inspector::(anonymous namespace)::ChannelImpl::sendResponse(int, std::unique_ptr<v8_inspector::StringBuffer, std::default_delete<v8_inspector::StringBuffer> >) (inspector_agent.cc:221)
    ==10848==    by 0x15C54EA: v8_inspector::V8InspectorSessionImpl::sendProtocolResponse(int, std::unique_ptr<v8_inspector::protocol::Serializable, std::default_delete<v8_inspector::protocol::Serializable> >) (v8-inspector-session-impl.cc:165)
    ==10848==    by 0x14C1E81: v8_inspector::protocol::DispatcherBase::sendResponse(int, v8_inspector::protocol::DispatchResponse const&, std::unique_ptr<v8_inspector::protocol::DictionaryValue, std::default_delete<v8_inspector::protocol::DictionaryValue> >) (Protocol.cpp:660)
    ==10848==    by 0x14C1F0A: v8_inspector::protocol::DispatcherBase::sendResponse(int, v8_inspector::protocol::DispatchResponse const&) (Protocol.cpp:665)
    ==10848==    by 0x14E68E3: v8_inspector::protocol::Debugger::DispatcherImpl::setAsyncCallStackDepth(int, std::unique_ptr<v8_inspector::protocol::DictionaryValue, std::default_delete<v8_inspector::protocol::DictionaryValue> >, v8_inspector::protocol::ErrorSupport*) (Debugger.cpp:1353)
    ==10848==    by 0x14E2D49: v8_inspector::protocol::Debugger::DispatcherImpl::dispatch(int, v8_inspector::String16 const&, std::unique_ptr<v8_inspector::protocol::DictionaryValue, std::default_delete<v8_inspector::protocol::DictionaryValue> >) (Debugger.cpp:920)
    ==10848==  Address 0x64e6f88 is 24 bytes inside a block of size 80 free'd
    ==10848==    at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==10848==    by 0x14534F8: node::inspector::(anonymous namespace)::JSBindingsConnection::~JSBindingsConnection() (inspector_js_api.cc:34)
    ==10848==    by 0x145187E: node::inspector::(anonymous namespace)::JSBindingsConnection::Disconnect() (inspector_js_api.cc:111)
    ==10848==    by 0x14518C9: node::inspector::(anonymous namespace)::JSBindingsConnection::Disconnect(v8::FunctionCallbackInfo<v8::Value> const&) (inspector_js_api.cc:117)
    ==10848==    by 0x166FF87: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (api-arguments.cc:26)
    ==10848==    by 0x172F829: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) (builtins-api.cc:112)
    ==10848==    by 0x172D85C: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (builtins-api.cc:142)
    ==10848==    by 0x172D5F6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) (builtins-api.cc:130)
    ==10848==    by 0x7895E1842C3: ???
    ==10848==    by 0x7895E19B737: ???
    ==10848==    by 0x7895E19B737: ???
    ==10848==    by 0x7895E18F9C2: ???
    ==10848==  Block was alloc'd at
    ==10848==    at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==10848==    by 0x14517E8: node::inspector::(anonymous namespace)::JSBindingsConnection::New(v8::FunctionCallbackInfo<v8::Value> const&) (inspector_js_api.cc:103)
    ==10848==    by 0x166FF87: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (api-arguments.cc:26)
    ==10848==    by 0x172F113: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) (builtins-api.cc:112)
    ==10848==    by 0x172D748: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (builtins-api.cc:138)
    ==10848==    by 0x172D5F6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) (builtins-api.cc:130)
    ==10848==    by 0x7895E1842C3: ???
    ==10848==    by 0x7895E1930DC: ???
    ==10848==    by 0x7895E293EAA: ???
    ==10848==    by 0x7895E19B737: ???
    ==10848==    by 0x7895E19B737: ???
    ==10848==    by 0x7895E19B737: ???
    [...]

PR-URL: #19381
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Author: addaleax

src/async_wrap.cc

@@ -202,13 +202,13 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) {
 }
 
 
-void AsyncWrap::EmitTraceEventAfter() {
-  switch (provider_type()) {
+void AsyncWrap::EmitTraceEventAfter(ProviderType type, double async_id) {
+  switch (type) {
 #define V(PROVIDER)                                                           \
     case PROVIDER_ ## PROVIDER:                                               \
       TRACE_EVENT_NESTABLE_ASYNC_END0(                                        \
         TRACING_CATEGORY_NODE1(async_hooks),                                  \
-        #PROVIDER "_CALLBACK", static_cast<int64_t>(get_async_id()));         \
+        #PROVIDER "_CALLBACK", static_cast<int64_t>(async_id));               \
       break;
     NODE_ASYNC_PROVIDER_TYPES(V)
 #undef V
@@ -314,7 +314,7 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
     wrap->EmitTraceEventBefore();
     AsyncWrap::EmitBefore(wrap->env(), wrap->get_async_id());
   } else if (type == PromiseHookType::kAfter) {
-    wrap->EmitTraceEventAfter();
+    wrap->EmitTraceEventAfter(wrap->provider_type(), wrap->get_async_id());
     AsyncWrap::EmitAfter(wrap->env(), wrap->get_async_id());
     if (env->execution_async_id() == wrap->get_async_id()) {
       // This condition might not be true if async_hooks was enabled during
@@ -701,11 +701,14 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
                                           Local<Value>* argv) {
   EmitTraceEventBefore();
 
+  ProviderType provider = provider_type();
   async_context context { get_async_id(), get_trigger_async_id() };
   MaybeLocal<Value> ret = InternalMakeCallback(
       env(), object(), cb, argc, argv, context);
 
-  EmitTraceEventAfter();
+  // This is a static call with cached values because the `this` object may
+  // no longer be alive at this point.
+  EmitTraceEventAfter(provider, context.async_id);
 
   return ret;
 }

src/async_wrap.h

@@ -142,7 +142,7 @@ class AsyncWrap : public BaseObject {
   static void EmitPromiseResolve(Environment* env, double async_id);
 
   void EmitTraceEventBefore();
-  void EmitTraceEventAfter();
+  static void EmitTraceEventAfter(ProviderType type, double async_id);
   void EmitTraceEventDestroy();