Skip to content

Commit

Permalink
history: don't handle db error during destruction
Browse files Browse the repository at this point in the history
Handling of db errors is delayed using a posttask. ~HistoryBackend
closes all the dbs. If closing the db results in an error, then
a PostTask() is scheduled with a HistoryBackend that is part way
through deletion. When the PostTask() runs, we get a uaf.

This patch resets the error callback in ~HistoryBackend to ensure
this doesn't happen. This means a db error is effectively ignored
during shutdown. Presumably if the error is fatal, it'll be handled
when the HistoryBackend is created again.

BUG=1306507
TEST=none

Change-Id: Ic158589a43e7bc2fd1f602fb2798ab500dc8d6d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3538001
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983478}
  • Loading branch information
Scott Violet authored and Chromium LUCI CQ committed Mar 21, 2022
1 parent 2eb9232 commit 72315bb
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
9 changes: 9 additions & 0 deletions components/history/core/browser/history_backend.cc
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,15 @@ HistoryBackend::~HistoryBackend() {
// Release stashed embedder object before cleaning up the databases.
supports_user_data_helper_.reset();

// Clear the error callback. The error callback that is installed does not
// process an error immediately, rather it uses a PostTask() with `this`. As
// `this` is being deleted, scheduling a PostTask() with `this` would be
// fatal (use-after-free). Additionally, as we're in shutdown, there isn't
// much point in trying to handle the error. If the error is really fatal,
// we'll cleanup the next time the backend is created.
if (db_)
db_->reset_error_callback();

// First close the databases before optionally running the "destroy" task.
CloseAllDatabases();

Expand Down
1 change: 1 addition & 0 deletions components/history/core/browser/history_database.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class HistoryDatabase : public DownloadDatabase,
void set_error_callback(const sql::Database::ErrorCallback& error_callback) {
db_.set_error_callback(error_callback);
}
void reset_error_callback() { db_.reset_error_callback(); }

// Must call this function to complete initialization. Will return
// sql::INIT_OK on success. Otherwise, no other function should be called. You
Expand Down

0 comments on commit 72315bb

Please sign in to comment.