Skip to content

fix: remove catch-all HandleScope#22531

Merged
nornagon merged 19 commits intomasterfrom
no-catchall-handle-scope
Mar 11, 2020
Merged

fix: remove catch-all HandleScope#22531
nornagon merged 19 commits intomasterfrom
no-catchall-handle-scope

Conversation

@nornagon
Copy link
Contributor

@nornagon nornagon commented Mar 4, 2020 •
edited
Loading

Description of Change

This removes the catch-all HandleScope in JavascriptEnvironment. This could cause memory leaks when creating v8 objects in C++ without an explicit HandleScope on the stack. Now, such usage will cause a crash.

Checklist

Release Notes

Notes: Fixed several memory leaks related to V8 handles not being properly scoped.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Mar 4, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Mar 5, 2020
@nornagon
Copy link
Contributor Author

nornagon commented Mar 6, 2020

Annotated crash from asan:

=================================================================
==5416==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160005daea0 at pc 0x558ebf0eced6 bp 0x7ffdcbe139c0 sp 0x7ffdcbe139b8
READ of size 8 at 0x6160005daea0 thread T0 (electron)
    #0 0x558ebf0eced5 in operator() ./../../electron/shell/common/gin_helper/wrappable.cc:82:53
    #1 0x558ebf0eced5 in Invoke<(lambda at ../../electron/shell/common/gin_helper/wrappable.cc:82:22), gin_helper::WrappableBase *> ./../../base/bind_internal.h:386:12
    #2 0x558ebf0eced5 in MakeItSo<(lambda at ../../electron/shell/common/gin_helper/wrappable.cc:82:22), gin_helper::WrappableBase *> ./../../base/bind_internal.h:599:12
    #3 0x558ebf0eced5 in RunImpl<(lambda at ../../electron/shell/common/gin_helper/wrappable.cc:82:22), std::__1::tuple<base::internal::UnretainedWrapper<gin_helper::WrappableBase> >, 0> ./../../base/bind_internal.h:672:12
    #4 0x558ebf0eced5 in base::internal::Invoker<base::internal::BindState<gin_helper::WrappableBase::SecondWeakCallback(v8::WeakCallbackInfo<gin_helper::WrappableBase> const&)::$_0, base::internal::UnretainedWrapper<gin_helper::WrappableBase> >, void ()>::RunOnce(base::internal::BindStateBase*) ./../../base/bind_internal.h:641:12
    #5 0x558ecb836123 in Run ./../../base/callback.h:98:12
    #6 0x558ecb836123 in base::TaskAnnotator::RunTask(char const*, base::PendingTask*) ./../../base/task/common/task_annotator.cc:142:33
    #7 0x558ecb8a37b6 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl(base::sequence_manager::LazyNow*, bool*) ./../../base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:407:23
    #8 0x558ecb8a2ea6 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoSomeWork() ./../../base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:247:7
    #9 0x558ecb738869 in HandleDispatch ./../../base/message_loop/message_pump_glib.cc:409:46
    #10 0x558ecb738869 in base::(anonymous namespace)::WorkSourceDispatch(_GSource*, int (*)(void*), void*) ./../../base/message_loop/message_pump_glib.cc:122:43
    #11 0x7fac819cf416 in g_main_context_dispatch ??:0:0

0x6160005daea0 is located 32 bytes inside of 632-byte region [0x6160005dae80,0x6160005db0f8)
freed by thread T0 (electron) here:
    #0 0x558ebeb36684 in operator delete(void*) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:160:3
    #1 0x558ebf0eb65e in Invoke<void (gin_helper::TrackableObjectBase::*)(), base::WeakPtr<gin_helper::TrackableObjectBase>> ./../../base/bind_internal.h:499:12
    #2 0x558ebf0eb65e in MakeItSo<void (gin_helper::TrackableObjectBase::*)(), base::WeakPtr<gin_helper::TrackableObjectBase>> ./../../base/bind_internal.h:619:5
    #3 0x558ebf0eb65e in RunImpl<void (gin_helper::TrackableObjectBase::*)(), std::__1::tuple<base::WeakPtr<gin_helper::TrackableObjectBase> >, 0> ./../../base/bind_internal.h:672:12
    #4 0x558ebf0eb65e in base::internal::Invoker<base::internal::BindState<void (gin_helper::TrackableObjectBase::*)(), base::WeakPtr<gin_helper::TrackableObjectBase> >, void ()>::RunOnce(base::internal::BindStateBase*) ./../../base/bind_internal.h:641:12
    #5 0x558ecb836123 in Run ./../../base/callback.h:98:12
    #6 0x558ecb836123 in base::TaskAnnotator::RunTask(char const*, base::PendingTask*) ./../../base/task/common/task_annotator.cc:142:33
    #7 0x558ecb8a37b6 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl(base::sequence_manager::LazyNow*, bool*) ./../../base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:407:23
    #8 0x558ecb8a2ea6 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoSomeWork() ./../../base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:247:7
    #9 0x558ecb737170 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*) ./../../base/message_loop/message_pump_glib.cc:443:48
    #10 0x558ecb8a5889 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(bool, base::TimeDelta) ./../../base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:512:12
    #11 0x558ecb7ce959 in base::RunLoop::Run() ./../../base/run_loop.cc:124:14
    #12 0x558ec7690c81 in MainMessageLoopRun ./../../content/browser/browser_main_loop.cc:1498:12
    #13 0x558ec7690c81 in content::BrowserMainLoop::RunMainMessageLoopParts() ./../../content/browser/browser_main_loop.cc:1058:5
    #14 0x558ec7698ecc in content::BrowserMainRunnerImpl::Run() ./../../content/browser/browser_main_runner_impl.cc:150:15
    #15 0x558ec768800d in content::BrowserMain(content::MainFunctionParams const&) ./../../content/browser/browser_main.cc:47:28
    #16 0x558ec6fec49e in RunBrowserProcessMain ./../../content/app/content_main_runner_impl.cc:529:10
    #17 0x558ec6fec49e in content::ContentMainRunnerImpl::RunServiceManager(content::MainFunctionParams&, bool) ./../../content/app/content_main_runner_impl.cc:977:10
    #18 0x558ec6feb5f2 in content::ContentMainRunnerImpl::Run(bool) ./../../content/app/content_main_runner_impl.cc:877:12
    #19 0x558ed17ced32 in service_manager::Main(service_manager::MainParams const&) ./../../services/service_manager/embedder/main.cc:423:29
    #20 0x558ec2e80dff in content::ContentMain(content::ContentMainParams const&) ./../../content/app/content_main.cc:19:10
    #21 0x558ebeb387cb in main ./../../electron/shell/app/electron_main.cc:179:10
    #22 0x7fac7ae32b96 in __libc_start_main ??:0:0

previously allocated by thread T0 (electron) here:
    #0 0x558ebeb35e84 in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:99:3
    #1 0x558ebeda6cb9 in electron::api::WebContents::Create(v8::Isolate*, gin_helper::Dictionary const&) ./../../electron/shell/browser/api/electron_api_web_contents.cc:2741:37
    #2 0x558ebebccc64 in electron::api::BrowserWindow::BrowserWindow(gin::Arguments*, gin_helper::Dictionary const&) ./../../electron/shell/browser/api/electron_api_browser_window.cc:77:20
    #3 0x558ebebd5bf1 in electron::api::BrowserWindow::New(gin_helper::ErrorThrower, gin::Arguments*) ./../../electron/shell/browser/api/electron_api_browser_window.cc:492:14
    #4 0x558ebebbfadc in Invoke<gin_helper::WrappableBase *(*const &)(gin_helper::ErrorThrower, gin::Arguments *), gin_helper::ErrorThrower, gin::Arguments *> ./../../base/bind_internal.h:399:12
    #5 0x558ebebbfadc in MakeItSo<gin_helper::WrappableBase *(*const &)(gin_helper::ErrorThrower, gin::Arguments *), gin_helper::ErrorThrower, gin::Arguments *> ./../../base/bind_internal.h:599:12
    #6 0x558ebebbfadc in RunImpl<gin_helper::WrappableBase *(*const &)(gin_helper::ErrorThrower, gin::Arguments *), const std::__1::tuple<> &> ./../../base/bind_internal.h:672:12
    #7 0x558ebebbfadc in base::internal::Invoker<base::internal::BindState<gin_helper::WrappableBase* (*)(gin_helper::ErrorThrower, gin::Arguments*)>, gin_helper::WrappableBase* (gin_helper::ErrorThrower, gin::Arguments*)>::Run(base::internal::BindStateBase*, gin_helper::ErrorThrower&&, gin::Arguments*) ./../../base/bind_internal.h:654:12
    #8 0x558ebebbf6b6 in Run ./../../base/callback.h:132:12
    #9 0x558ebebbf6b6 in gin_helper::WrappableBase* gin_helper::internal::InvokeFactory<gin_helper::ErrorThrower, gin::Arguments*>(gin::Arguments*, base::RepeatingCallback<gin_helper::WrappableBase* (gin_helper::ErrorThrower, gin::Arguments*)> const&) ./../../electron/shell/common/gin_helper/constructor.h:43:19
    #10 0x558ebebbe8df in void gin_helper::internal::InvokeNew<gin_helper::WrappableBase* (gin_helper::ErrorThrower, gin::Arguments*)>(base::RepeatingCallback<gin_helper::WrappableBase* (gin_helper::ErrorThrower, gin::Arguments*)> const&, v8::Isolate*, gin_helper::Arguments*) ./../../electron/shell/common/gin_helper/constructor.h:132:14
    #11 0x558ebebbef1a in Run ./../../base/callback.h:132:12
    #12 0x558ebebbef1a in DispatchToCallback ./../../electron/shell/common/gin_helper/function_template.h:229:14
    #13 0x558ebebbef1a in gin_helper::Dispatcher<void (v8::Isolate*, gin_helper::Arguments*)>::DispatchToCallback(v8::FunctionCallbackInfo<v8::Value> const&) ./../../electron/shell/common/gin_helper/function_template.h:263:15
    #14 0x558ec3a93a05 in v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) ./../../v8/src/api/api-arguments-inl.h:158:3
    #15 0x558ec3a8eb5c in 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) ./../../v8/src/builtins/builtins-api.cc:111:36
    #16 0x558ec3a8c1cd in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) ./../../v8/src/builtins/builtins-api.cc:137:5
    #17 0x558ec3a8b32c in v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) ./../../v8/src/builtins/builtins-api.cc:129:1
    #18 0x558ec64c821e in Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit ??:0:0
    #19 0x558ec62a0d44 in Builtins_JSBuiltinsConstructStub ??:0:0
    #20 0x558ec673f485 in Builtins_ConstructHandler ??:0:0
    #21 0x558ec62b0cf0 in Builtins_InterpreterEntryTrampoline ??:0:0
    #22 0x558ec62b0cf0 in Builtins_InterpreterEntryTrampoline ??:0:0
    #23 0x558ec62b0cf0 in Builtins_InterpreterEntryTrampoline ??:0:0
    #24 0x558ec62b0cf0 in Builtins_InterpreterEntryTrampoline ??:0:0
    #25 0x558ec62b0cf0 in Builtins_InterpreterEntryTrampoline ??:0:0
    #26 0x558ec62b0cf0 in Builtins_InterpreterEntryTrampoline ??:0:0
    #27 0x558ec62a78f9 in Builtins_JSEntryTrampoline ??:0:0
    #28 0x558ec62a76d7 in Builtins_JSEntry ??:0:0
    #29 0x558ec3e16175 in Call ./../../v8/src/execution/simulator.h:142:12
    #30 0x558ec3e16175 in v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) ./../../v8/src/execution/execution.cc:367:33
    #31 0x558ec3e15055 in v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) ./../../v8/src/execution/execution.cc:461:10
    #32 0x558ec387f1b9 in v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) ./../../v8/src/api/api.cc:4989:7
    #33 0x558edd324333 in node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) ./../../third_party/electron_node/src/api/callback.cc:163:21
    #34 0x558edd324ac6 in node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) ./../../third_party/electron_node/src/api/callback.cc:229:7
    #35 0x558edd3c53b7 in node::Environment::CheckImmediate(uv_check_s*) ./../../third_party/electron_node/src/env.cc:795:5
    #36 0x558edd8489a3 in uv__run_check ./../../third_party/electron_node/deps/uv/src/unix/loop-watcher.c:67:1

SUMMARY: AddressSanitizer: heap-use-after-free (/home/ubuntu/tmp/electron+0xf573ed5)
Shadow bytes around the buggy address:
  0x0c2c800b3580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c800b3590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c800b35a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c800b35b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c800b35c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2c800b35d0: fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800b35e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800b35f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800b3600: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800b3610: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c2c800b3620: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==5416==ABORTING

@nornagon nornagon force-pushed the no-catchall-handle-scope branch from 4a7075d to bacba55 Compare March 7, 2020 00:20
@nornagon nornagon requested a review from zcbenz March 7, 2020 00:20
@nornagon
Copy link
Contributor Author

nornagon commented Mar 7, 2020

@zcbenz it looks like this triggered a UAF related to the "running JS in the WebContents destructor" patch you made earlier. I'm not exactly sure yet what the right way to resolve it is, but you might have some insight.

@zcbenz
Copy link
Contributor

zcbenz commented Mar 7, 2020

@nornagon Current master is crashing when running tests after last Chromium upgrade due to memory corruption, for example https://circleci.com/gh/electron/electron/486317?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link.

We should probably solve it first and then see if this branch still crashes.

I looked into the crash yesterday but did not make much progress, I was hoping #22514 to make things better.

zcbenz
zcbenz approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nornagon
Copy link
Contributor Author

nornagon commented Mar 11, 2020

There are probably a few remaining things that tests failed to catch, but hopefully we'll pick them up during alpha/beta.

@nornagon nornagon merged commit 19314d3 into master Mar 11, 2020
@release-clerk
Copy link

release-clerk bot commented Mar 11, 2020

Release Notes Persisted

Fixed several memory leaks related to V8 handles not being properly scoped.

@nornagon nornagon deleted the no-catchall-handle-scope branch March 11, 2020 01:17
This was referenced Mar 12, 2020
Merged
Merged
@zcbenz zcbenz mentioned this pull request Mar 19, 2020
Merged
@codebytere codebytere mentioned this pull request Mar 26, 2020
Merged
4 tasks
This was referenced Apr 14, 2020
Closed
Merged
@ckerr ckerr mentioned this pull request Apr 30, 2020
Closed
4 tasks
@nornagon nornagon mentioned this pull request May 19, 2020
Merged
@codebytere codebytere mentioned this pull request Jun 3, 2020
Merged
5 tasks
@deepak1556 deepak1556 mentioned this pull request Jul 28, 2020
Merged
@zeeker999 zeeker999 mentioned this pull request Sep 8, 2020
Closed
3 tasks
@nornagon nornagon mentioned this pull request Sep 8, 2020
Merged
5 tasks
@deepak1556 deepak1556 mentioned this pull request Nov 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zcbenz zcbenz zcbenz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nornagon @zcbenz