[sql] Prevent recovery against a closed database.

The referenced bug is a crash which appears to happen when recovery is
attempted against a poisoned (or closed) database.  Modify
sql::Recovery::Begin() to explicitly short-circuit recovery if the
passed connection handle is not open.  Otherwise the recovery code will
work until the recovered data is restored over the original, and then
fail.

Also modify the sql::Connection::ReportDiagnosticInfo() to clear the
error callback while doing any integrity check.  This is a bit of a
hack in the interests of not rooting out the crazy error-callback
assumptions at this time.

BUG=583106

Review URL: https://codereview.chromium.org/1657593005

Cr-Commit-Position: refs/heads/master@{#372896}

3 files changed, 29 insertions, 0 deletions

diff --git a/sql/connection.cc b/sql/connection.cc
index f08560c..f94e7ab 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -268,7 +268,15 @@ void Connection::ReportDiagnosticInfo(int extended_error, Statement* stmt) {
   std::string debug_info;
   const int error = (extended_error & 0xFF);
   if (error == SQLITE_CORRUPT) {
+    // CollectCorruptionInfo() is implemented in terms of sql::Connection,
+    // prevent reentrant calls to the error callback.
+    // TODO(shess): Rewrite IntegrityCheckHelper() in terms of raw SQLite.
+    ErrorCallback original_callback = std::move(error_callback_);
+    reset_error_callback();
+
     debug_info = CollectCorruptionInfo();
+
+    error_callback_ = std::move(original_callback);
   } else {
     debug_info = CollectErrorInfo(extended_error, stmt);
   }
diff --git a/sql/recovery.cc b/sql/recovery.cc
index 377aafb..5e101b1 100644
--- a/sql/recovery.cc
+++ b/sql/recovery.cc
@@ -108,6 +108,16 @@ bool Recovery::FullRecoverySupported() {
 scoped_ptr<Recovery> Recovery::Begin(
     Connection* connection,
     const base::FilePath& db_path) {
+  // Recovery is likely to be used in error handling.  Since recovery changes
+  // the state of the handle, protect against multiple layers attempting the
+  // same recovery.
+  if (!connection->is_open()) {
+    // Warn about API mis-use.
+    DLOG_IF(FATAL, !connection->poisoned_)
+        << "Illegal to recover with closed database";
+    return scoped_ptr<Recovery>();
+  }
+
   scoped_ptr<Recovery> r(new Recovery(connection));
   if (!r->Init(db_path)) {
     // TODO(shess): Should Init() failure result in Raze()?
diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc
index f964eb9..11f2a87 100644
--- a/sql/recovery_unittest.cc
+++ b/sql/recovery_unittest.cc
@@ -103,6 +103,17 @@ TEST_F(SQLRecoveryTest, RecoverBasic) {
   EXPECT_TRUE(db().is_open());
   ASSERT_EQ("", GetSchema(&db()));
 
+  // Attempting to recover a previously-recovered handle fails early.
+  {
+    scoped_ptr<sql::Recovery> recovery = sql::Recovery::Begin(&db(), db_path());
+    ASSERT_TRUE(recovery.get());
+    recovery.reset();
+
+    recovery = sql::Recovery::Begin(&db(), db_path());
+    ASSERT_FALSE(recovery.get());
+  }
+  ASSERT_TRUE(Reopen());
+
   // Recreate the database.
   ASSERT_TRUE(db().Execute(kCreateSql));
   ASSERT_TRUE(db().Execute(kInsertSql));