From 1a14f497bd17d41d0e0ffceb1fb23dea507b8eae Mon Sep 17 00:00:00 2001 From: sky Date: Tue, 26 Aug 2014 14:44:26 -0700 Subject: [PATCH] Fixes possible use after free in SessionService SessionService::GetLastSession used a base::Unretained but there was no guarantee that the SessionService would be valid by the time the callback was processed. BUG=399655 TEST=covered by test now R=marja@chromium.org Review URL: https://codereview.chromium.org/500143002 Cr-Commit-Position: refs/heads/master@{#291985} --- chrome/browser/sessions/base_session_service.cc | 7 +-- chrome/browser/sessions/base_session_service.h | 2 +- chrome/browser/sessions/session_service.cc | 8 ++-- chrome/browser/sessions/session_service.h | 3 ++ .../sessions/session_service_test_helper.cc | 6 +++ .../browser/sessions/session_service_test_helper.h | 8 ++++ .../browser/sessions/session_service_unittest.cc | 53 +++++++++++++++++++++- 7 files changed, 76 insertions(+), 11 deletions(-) diff --git a/chrome/browser/sessions/base_session_service.cc b/chrome/browser/sessions/base_session_service.cc index a130c89d2ea5..c7e6e7a113e1 100644 --- a/chrome/browser/sessions/base_session_service.cc +++ b/chrome/browser/sessions/base_session_service.cc @@ -294,20 +294,17 @@ BaseSessionService::ScheduleGetLastSessionCommands( return id; } -bool BaseSessionService::RunTaskOnBackendThread( +void BaseSessionService::RunTaskOnBackendThread( const tracked_objects::Location& from_here, const base::Closure& task) { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool(); if (!pool->IsShutdownInProgress()) { - return pool->PostSequencedWorkerTask(sequence_token_, - from_here, - task); + pool->PostSequencedWorkerTask(sequence_token_, from_here, task); } else { // Fall back to executing on the main thread if the sequence // worker pool has been requested to shutdown (around shutdown // time). task.Run(); - return true; } } diff --git a/chrome/browser/sessions/base_session_service.h b/chrome/browser/sessions/base_session_service.h index e85799a66e6b..98f237955ac9 100644 --- a/chrome/browser/sessions/base_session_service.h +++ b/chrome/browser/sessions/base_session_service.h @@ -156,7 +156,7 @@ class BaseSessionService { // This posts the task to the SequencedWorkerPool, or run immediately // if the SequencedWorkerPool has been shutdown. - bool RunTaskOnBackendThread(const tracked_objects::Location& from_here, + void RunTaskOnBackendThread(const tracked_objects::Location& from_here, const base::Closure& task); // Max number of navigation entries in each direction we'll persist. diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc index 7b36c75f8efd..0a0ac5a6c652 100644 --- a/chrome/browser/sessions/session_service.cc +++ b/chrome/browser/sessions/session_service.cc @@ -234,7 +234,8 @@ SessionService::SessionService(Profile* profile) save_delay_in_millis_(base::TimeDelta::FromMilliseconds(2500)), save_delay_in_mins_(base::TimeDelta::FromMinutes(10)), save_delay_in_hrs_(base::TimeDelta::FromHours(8)), - force_browser_not_alive_with_no_windows_(false) { + force_browser_not_alive_with_no_windows_(false), + weak_factory_(this) { Init(); } @@ -245,7 +246,8 @@ SessionService::SessionService(const base::FilePath& save_path) save_delay_in_millis_(base::TimeDelta::FromMilliseconds(2500)), save_delay_in_mins_(base::TimeDelta::FromMinutes(10)), save_delay_in_hrs_(base::TimeDelta::FromHours(8)), - force_browser_not_alive_with_no_windows_(false) { + force_browser_not_alive_with_no_windows_(false), + weak_factory_(this) { Init(); } @@ -571,7 +573,7 @@ base::CancelableTaskTracker::TaskId SessionService::GetLastSession( // the callback. return ScheduleGetLastSessionCommands( base::Bind(&SessionService::OnGotSessionCommands, - base::Unretained(this), callback), + weak_factory_.GetWeakPtr(), callback), tracker); } diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h index 10c337ddadcc..8720d27385a8 100644 --- a/chrome/browser/sessions/session_service.h +++ b/chrome/browser/sessions/session_service.h @@ -11,6 +11,7 @@ #include "base/basictypes.h" #include "base/callback.h" #include "base/memory/scoped_vector.h" +#include "base/memory/weak_ptr.h" #include "base/task/cancelable_task_tracker.h" #include "base/time/time.h" #include "chrome/browser/defaults.h" @@ -493,6 +494,8 @@ class SessionService : public BaseSessionService, // without quitting. bool force_browser_not_alive_with_no_windows_; + base::WeakPtrFactory weak_factory_; + DISALLOW_COPY_AND_ASSIGN(SessionService); }; diff --git a/chrome/browser/sessions/session_service_test_helper.cc b/chrome/browser/sessions/session_service_test_helper.cc index e9ee6da28fe8..0a1f98289f14 100644 --- a/chrome/browser/sessions/session_service_test_helper.cc +++ b/chrome/browser/sessions/session_service_test_helper.cc @@ -5,6 +5,7 @@ #include "chrome/browser/sessions/session_service_test_helper.h" #include "base/memory/scoped_vector.h" +#include "base/message_loop/message_loop.h" #include "chrome/browser/sessions/session_backend.h" #include "chrome/browser/sessions/session_service.h" #include "chrome/browser/sessions/session_types.h" @@ -111,3 +112,8 @@ void SessionServiceTestHelper::SetService(SessionService* service) { content::BrowserThread::GetBlockingPool()->FlushForTesting(); } +void SessionServiceTestHelper::RunTaskOnBackendThread( + const tracked_objects::Location& from_here, + const base::Closure& task) { + service_->RunTaskOnBackendThread(from_here, task); +} diff --git a/chrome/browser/sessions/session_service_test_helper.h b/chrome/browser/sessions/session_service_test_helper.h index 7f176c727bec..8daadd8e5773 100644 --- a/chrome/browser/sessions/session_service_test_helper.h +++ b/chrome/browser/sessions/session_service_test_helper.h @@ -10,6 +10,7 @@ #include "base/basictypes.h" #include "base/memory/scoped_ptr.h" +#include "base/message_loop/message_loop.h" #include "components/sessions/session_id.h" class SessionBackend; @@ -18,6 +19,10 @@ class SessionService; struct SessionTab; struct SessionWindow; +namespace base { +class RunLoop; +} + namespace sessions { class SerializedNavigationEntry; } @@ -76,6 +81,9 @@ class SessionServiceTestHelper { SessionBackend* backend(); + void RunTaskOnBackendThread(const tracked_objects::Location& from_here, + const base::Closure& task); + private: scoped_ptr service_; diff --git a/chrome/browser/sessions/session_service_unittest.cc b/chrome/browser/sessions/session_service_unittest.cc index a4f2a44ba2eb..67836f19f8d0 100644 --- a/chrome/browser/sessions/session_service_unittest.cc +++ b/chrome/browser/sessions/session_service_unittest.cc @@ -9,9 +9,11 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" #include "base/path_service.h" +#include "base/run_loop.h" #include "base/stl_util.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" +#include "base/synchronization/waitable_event.h" #include "base/time/time.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/chrome_notification_types.h" @@ -164,8 +166,6 @@ class SessionServiceTest : public BrowserWithTestWindowTest, SessionService* service() { return helper_.service(); } - SessionBackend* backend() { return helper_.backend(); } - const gfx::Rect window_bounds; SessionID window_id; @@ -999,3 +999,52 @@ TEST_F(SessionServiceTest, IgnoreBlacklistedUrls) { helper_.AssertTabEquals(window_id, tab_id, 0, 0, 1, *tab); helper_.AssertNavigationEquals(nav1, tab->navigations[0]); } + +// Functions used by GetSessionsAndDestroy. +namespace { + +void OnGotPreviousSession(ScopedVector windows, + SessionID::id_type ignored_active_window) { + FAIL() << "SessionService was destroyed, this shouldn't be reached."; +} + +void PostBackToThread(base::MessageLoop* message_loop, + base::RunLoop* run_loop) { + message_loop->PostTask(FROM_HERE, + base::Bind(&base::RunLoop::Quit, + base::Unretained(run_loop))); +} + +} // namespace + +// Verifies that SessionService::GetLastSession() works correctly if the +// SessionService is deleted during processing. To verify the problematic case +// does the following: +// 1. Sends a task to the background thread that blocks. +// 2. Asks SessionService for the last session commands. This is blocked by 1. +// 3. Posts another task to the background thread, this too is blocked by 1. +// 4. Deletes SessionService. +// 5. Signals the semaphore that 2 and 3 are waiting on, allowing +// GetLastSession() to continue. +// 6. runs the message loop, this is quit when the task scheduled in 3 posts +// back to the ui thread to quit the run loop. +// The call to get the previous session should never be invoked because the +// SessionService was destroyed before SessionService could process the results. +TEST_F(SessionServiceTest, GetSessionsAndDestroy) { + base::CancelableTaskTracker cancelable_task_tracker; + base::RunLoop run_loop; + base::WaitableEvent event(true, false); + helper_.RunTaskOnBackendThread(FROM_HERE, + base::Bind(&base::WaitableEvent::Wait, + base::Unretained(&event))); + service()->GetLastSession(base::Bind(&OnGotPreviousSession), + &cancelable_task_tracker); + helper_.RunTaskOnBackendThread( + FROM_HERE, + base::Bind(&PostBackToThread, + base::Unretained(base::MessageLoop::current()), + base::Unretained(&run_loop))); + delete helper_.ReleaseService(); + event.Signal(); + run_loop.Run(); +} -- 2.11.4.GIT