Skip to content

Commit

Permalink
async_wrap: fix use-after-free for inspector session
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
  • Loading branch information
@addaleax
addaleax committed Mar 30, 2018
1 parent d37e59f commit abc8786
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 6 deletions.
13 changes: 8 additions & 5 deletions src/async_wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/async_wrap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();


Expand Down

0 comments on commit abc8786

Please sign in to comment.