From abc87862ff14c1571f008aa1a9cbf812bea9790c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 16 Mar 2018 00:48:34 +0100 Subject: 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, int, v8::Local*) (async_wrap.cc:724) ==10848== by 0x14516C6: node::inspector::(anonymous namespace)::JSBindingsConnection::OnMessage(v8::Local) (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 >) (inspector_agent.cc:221) ==10848== by 0x15C54EA: v8_inspector::V8InspectorSessionImpl::sendProtocolResponse(int, std::unique_ptr >) (v8-inspector-session-impl.cc:165) ==10848== by 0x14C1E81: v8_inspector::protocol::DispatcherBase::sendResponse(int, v8_inspector::protocol::DispatchResponse const&, std::unique_ptr >) (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::ErrorSupport*) (Debugger.cpp:1353) ==10848== by 0x14E2D49: v8_inspector::protocol::Debugger::DispatcherImpl::dispatch(int, v8_inspector::String16 const&, std::unique_ptr >) (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 const&) (inspector_js_api.cc:117) ==10848== by 0x166FF87: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo const&)) (api-arguments.cc:26) ==10848== by 0x172F829: v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate*, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, 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 const&) (inspector_js_api.cc:103) ==10848== by 0x166FF87: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo const&)) (api-arguments.cc:26) ==10848== by 0x172F113: v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate*, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, 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: https://github.com/nodejs/node/pull/19381 Reviewed-By: Eugene Ostroukhov Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Tiancheng "Timothy" Gu --- src/async_wrap.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'src/async_wrap.cc') diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 90b532d73b..de24239dd9 100644 --- a/src/async_wrap.cc +++ b/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(get_async_id())); \ + #PROVIDER "_CALLBACK", static_cast(async_id)); \ break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V @@ -314,7 +314,7 @@ static void PromiseHook(PromiseHookType type, Local 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 AsyncWrap::MakeCallback(const Local cb, Local* argv) { EmitTraceEventBefore(); + ProviderType provider = provider_type(); async_context context { get_async_id(), get_trigger_async_id() }; MaybeLocal 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; } -- cgit v1.2.3