diff options
author | Anna Henningsen <anna@addaleax.net> | 2018-03-16 02:48:34 +0300 |
---|---|---|
committer | Anna Henningsen <anna@addaleax.net> | 2018-03-30 14:53:35 +0300 |
commit | abc87862ff14c1571f008aa1a9cbf812bea9790c (patch) | |
tree | 8489ea27a530e1ddc5faf98f869a7e62f532b5f0 /src/async_wrap.cc | |
parent | d37e59fa6aee7f5b38696726b0145741ef3eb95b (diff) |
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: https://github.com/nodejs/node/pull/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>
Diffstat (limited to 'src/async_wrap.cc')
-rw-r--r-- | src/async_wrap.cc | 13 |
1 files changed, 8 insertions, 5 deletions
diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 90b532d73b8..de24239dd95 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<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; } |