From c04a5788210c4762afb6926eea1dd5611f0904b9 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Wed, 7 Apr 2021 16:58:43 -0400 Subject: [PATCH] Avoid potential use-after-free of CefIOThreadState (see issue #2969) The problem occured while executing multiple URLRequestTest with the Chrome runtime. --- libcef/browser/browser_context.cc | 58 +++++-------------- libcef/browser/browser_context.h | 8 ++- libcef/browser/iothread_state.cc | 4 +- libcef/browser/iothread_state.h | 13 ++++- .../resource_request_handler_wrapper.cc | 4 +- 5 files changed, 35 insertions(+), 52 deletions(-) diff --git a/libcef/browser/browser_context.cc b/libcef/browser/browser_context.cc index 956edf3a6..d0cefec41 100644 --- a/libcef/browser/browser_context.cc +++ b/libcef/browser/browser_context.cc @@ -7,7 +7,6 @@ #include #include -#include "libcef/browser/iothread_state.h" #include "libcef/browser/media_router/media_router_manager.h" #include "libcef/browser/request_context_impl.h" #include "libcef/browser/thread_util.h" @@ -156,13 +155,6 @@ CefBrowserContext::~CefBrowserContext() { #if DCHECK_IS_ON() DCHECK(is_shutdown_); #endif - - if (iothread_state_) { - // Destruction of the CefIOThreadState will trigger destruction of all - // associated network requests. - content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE, - iothread_state_.release()); - } } void CefBrowserContext::Initialize() { @@ -171,7 +163,7 @@ void CefBrowserContext::Initialize() { if (!cache_path_.empty()) g_manager.Get().SetImplPath(this, cache_path_); - iothread_state_ = std::make_unique(); + iothread_state_ = base::MakeRefCounted(); } void CefBrowserContext::Shutdown() { @@ -275,15 +267,10 @@ void CefBrowserContext::OnRenderFrameCreated( handler_map_.AddHandler(render_process_id, render_frame_id, frame_tree_node_id, handler); - if (iothread_state_) { - // Using base::Unretained() is safe because both this callback and - // possible deletion of |iothread_state_| will execute on the IO thread, - // and this callback will be executed first. - CEF_POST_TASK(CEF_IOT, base::Bind(&CefIOThreadState::AddHandler, - base::Unretained(iothread_state_.get()), - render_process_id, render_frame_id, - frame_tree_node_id, handler)); - } + CEF_POST_TASK(CEF_IOT, + base::Bind(&CefIOThreadState::AddHandler, iothread_state_, + render_process_id, render_frame_id, + frame_tree_node_id, handler)); } } @@ -313,15 +300,9 @@ void CefBrowserContext::OnRenderFrameDeleted( handler_map_.RemoveHandler(render_process_id, render_frame_id, frame_tree_node_id); - if (iothread_state_) { - // Using base::Unretained() is safe because both this callback and - // possible deletion of |iothread_state_| will execute on the IO thread, - // and this callback will be executed first. - CEF_POST_TASK(CEF_IOT, base::Bind(&CefIOThreadState::RemoveHandler, - base::Unretained(iothread_state_.get()), - render_process_id, render_frame_id, - frame_tree_node_id)); - } + CEF_POST_TASK(CEF_IOT, base::Bind(&CefIOThreadState::RemoveHandler, + iothread_state_, render_process_id, + render_frame_id, frame_tree_node_id)); } if (is_main_frame) { @@ -425,26 +406,15 @@ void CefBrowserContext::RegisterSchemeHandlerFactory( const CefString& scheme_name, const CefString& domain_name, CefRefPtr factory) { - if (iothread_state_) { - // Using base::Unretained() is safe because both this callback and possible - // deletion of |iothread_state_| will execute on the IO thread, and this - // callback will be executed first. - CEF_POST_TASK(CEF_IOT, - base::Bind(&CefIOThreadState::RegisterSchemeHandlerFactory, - base::Unretained(iothread_state_.get()), - scheme_name, domain_name, factory)); - } + CEF_POST_TASK(CEF_IOT, + base::Bind(&CefIOThreadState::RegisterSchemeHandlerFactory, + iothread_state_, scheme_name, domain_name, factory)); } void CefBrowserContext::ClearSchemeHandlerFactories() { - if (iothread_state_) { - // Using base::Unretained() is safe because both this callback and possible - // deletion of |iothread_state_| will execute on the IO thread, and this - // callback will be executed first. - CEF_POST_TASK(CEF_IOT, - base::Bind(&CefIOThreadState::ClearSchemeHandlerFactories, - base::Unretained(iothread_state_.get()))); - } + CEF_POST_TASK(CEF_IOT, + base::Bind(&CefIOThreadState::ClearSchemeHandlerFactories, + iothread_state_)); } void CefBrowserContext::LoadExtension( diff --git a/libcef/browser/browser_context.h b/libcef/browser/browser_context.h index 7c8e7d0a7..75cbd1534 100644 --- a/libcef/browser/browser_context.h +++ b/libcef/browser/browser_context.h @@ -10,6 +10,7 @@ #include #include "include/cef_request_context_handler.h" +#include "libcef/browser/iothread_state.h" #include "libcef/browser/request_context_handler_map.h" #include "base/callback.h" @@ -80,7 +81,6 @@ class BrowserContext; class CefMediaRouterManager; class CefRequestContextImpl; -class CefIOThreadState; class Profile; // Main entry point for configuring behavior on a per-RequestContext basis. The @@ -208,7 +208,9 @@ class CefBrowserContext { // change during this object's lifespan. const CefRequestContextSettings& settings() const { return settings_; } base::FilePath cache_path() const { return cache_path_; } - CefIOThreadState* iothread_state() const { return iothread_state_.get(); } + scoped_refptr iothread_state() const { + return iothread_state_; + } // Used to hold a WeakPtr reference to this this object. The Getter returns // nullptr if this object has already been destroyed. @@ -231,7 +233,7 @@ class CefBrowserContext { base::FilePath cache_path_; private: - std::unique_ptr iothread_state_; + scoped_refptr iothread_state_; CookieableSchemes cookieable_schemes_; std::unique_ptr media_router_manager_; diff --git a/libcef/browser/iothread_state.cc b/libcef/browser/iothread_state.cc index 6cea106f8..0ecec6491 100644 --- a/libcef/browser/iothread_state.cc +++ b/libcef/browser/iothread_state.cc @@ -23,7 +23,9 @@ CefIOThreadState::CefIOThreadState() { base::Unretained(this))); } -CefIOThreadState::~CefIOThreadState() {} +CefIOThreadState::~CefIOThreadState() { + CEF_REQUIRE_IOT(); +} void CefIOThreadState::AddHandler(int render_process_id, int render_frame_id, diff --git a/libcef/browser/iothread_state.h b/libcef/browser/iothread_state.h index 7c738697a..c0a6784de 100644 --- a/libcef/browser/iothread_state.h +++ b/libcef/browser/iothread_state.h @@ -12,15 +12,18 @@ #include "libcef/browser/request_context_handler_map.h" +#include "content/public/browser/browser_thread.h" + class GURL; // Stores state that will be accessed on the IO thread. Life span is controlled // by CefBrowserContext. Created on the UI thread but accessed and destroyed on // the IO thread. See browser_context.h for an object relationship diagram. -class CefIOThreadState { +class CefIOThreadState : public base::RefCountedThreadSafe< + CefIOThreadState, + content::BrowserThread::DeleteOnIOThread> { public: CefIOThreadState(); - virtual ~CefIOThreadState(); // See comments in CefRequestContextHandlerMap. void AddHandler(int render_process_id, @@ -44,6 +47,12 @@ class CefIOThreadState { CefRefPtr GetSchemeHandlerFactory(const GURL& url); private: + friend struct content::BrowserThread::DeleteOnThread< + content::BrowserThread::IO>; + friend class base::DeleteHelper; + + ~CefIOThreadState(); + void InitOnIOThread(); // Map IDs to CefRequestContextHandler objects. diff --git a/libcef/browser/net_service/resource_request_handler_wrapper.cc b/libcef/browser/net_service/resource_request_handler_wrapper.cc index 2dd430e02..64eb4896c 100644 --- a/libcef/browser/net_service/resource_request_handler_wrapper.cc +++ b/libcef/browser/net_service/resource_request_handler_wrapper.cc @@ -264,7 +264,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { auto profile = Profile::FromBrowserContext(browser_context); auto cef_browser_context = CefBrowserContext::FromProfile(profile); iothread_state_ = cef_browser_context->iothread_state(); - DCHECK(iothread_state_); + CHECK(iothread_state_); cookieable_schemes_ = cef_browser_context->GetCookieableSchemes(); // We register to be notified of CEF context or browser destruction so @@ -314,7 +314,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { CefRefPtr browser_; CefRefPtr frame_; - CefIOThreadState* iothread_state_ = nullptr; + scoped_refptr iothread_state_; CefBrowserContext::CookieableSchemes cookieable_schemes_; int render_process_id_ = 0; int render_frame_id_ = -1;