Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_wrap: fix use-after-free for inspector session #19381

Closed

Conversation

addaleax
Copy link
Member

This fixes the following condition:

valgrind output in the fold
$ 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: ???
[...]
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

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: ???
    [...]
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 15, 2018
@addaleax addaleax added async_wrap inspector Issues and PRs related to the V8 inspector protocol labels Mar 16, 2018
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2018
@addaleax
Copy link
Member Author

@gibfahn
Copy link
Member

gibfahn commented Mar 26, 2018

Rerunning the AIX job (only failure in the previous CI): https://ci.nodejs.org/job/node-test-commit-aix/13625/

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 30, 2018
@addaleax
Copy link
Member Author

Landed in abc8786

@addaleax addaleax closed this Mar 30, 2018
@addaleax addaleax deleted the async-inspector-use-after-free branch March 30, 2018 12:09
addaleax added a commit that referenced this pull request Mar 30, 2018
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>
@targos
Copy link
Member

targos commented Apr 2, 2018

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants