[sql] RecoverDatabase() deletes SQLITE_NOTADB databases.

sql/recovery.cc

Index: sql/recovery.cc
diff --git a/sql/recovery.cc b/sql/recovery.cc
index 5d5700262c2b71354908c9c4a0861c3e85be4e11..0bda2b5f7c65b2f171f9e805aa02c1f4f2ab77ac 100644
--- a/sql/recovery.cc
+++ b/sql/recovery.cc
@@ -108,6 +108,21 @@ enum RecoveryEventType {
   // Failed to recover triggers or views or virtual tables.
   RECOVERY_FAILED_AUTORECOVERDB_AUX,
 
+  // After SQLITE_NOTADB failure setting up for recovery, Delete() failed.
+  RECOVERY_FAILED_AUTORECOVERDB_NOTADB_DELETE,
+
+  // After SQLITE_NOTADB failure setting up for recovery, Delete() succeeded
+  // then Open() failed.
+  RECOVERY_FAILED_AUTORECOVERDB_NOTADB_REOPEN,
+
+  // After SQLITE_NOTADB failure setting up for recovery, Delete() and Open()
+  // succeeded, then querying the database failed.

[pwnall, 2017/02/27 22:28:33]

Based on some of the comments below, do you need a "Should be impossible."
comment here?

[Scott Hess - ex-Googler, 2017/02/27 23:39:39]

I don't know.  Substantially all of the FAILED events in this list should be
impossible.  Almost all of them actually happen, because by the time this code
is executing, something is _definitely_ wrong.  One approach would be to spend
the time writing handling code for all possible failures, but that has potential
to add a lot of complexity.  Instead, as I push handling into new areas, I also
push in coverage histograms to get a sense of which errors actually happen in
the fleet.

So I'm not sure if "Should be impossible" wouldn't be akin to comments like
"Adds one to variable |i|", where the comment is true, but not adding anything.

+  RECOVERY_FAILED_AUTORECOVERDB_NOTADB_QUERY,
+
+  // After SQLITE_NOTADB failure setting up for recovery, the database was
+  // successfully deleted.
+  RECOVERY_SUCCESS_AUTORECOVERDB_NOTADB_DELETE,
+
   // Always keep this at the end.
   RECOVERY_EVENT_MAX,
 };
@@ -588,9 +603,43 @@ void Recovery::RecoverDatabase(Connection* db,
                                const base::FilePath& db_path) {
   std::unique_ptr<sql::Recovery> recovery = sql::Recovery::Begin(db, db_path);
   if (!recovery) {
-    // TODO(shess): If recovery can't even get started, Raze() or Delete().
-    RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_BEGIN);
-    db->Poison();
+    // |db| was poisoned by Begin().  Probe for SQLITE_NOTADB when attaching to

[pwnall, 2017/02/27 22:28:33]

Is it possible to DCHECK that db was poisoned?

[Scott Hess - ex-Googler, 2017/02/27 23:39:39]

The comment was only there because it's a non-obvious part of the Begin() API
contract.  Since recovery can be destructive, the goal is to prevent
accidentally running queries against a database in an indeterminate state.

I think I'll just add an explicit call to Poison(), plus a comment about why.

[pwnall, 2017/02/27 23:54:51]

I'd have been fine with a DCHECK... it's like a comment that makes sure it stays
valid :D Either way works for me, you don't need to make a change here.

+    // db_path.
+    {
+      Connection probe_db;
+      if (!probe_db.OpenInMemory() ||
+          probe_db.AttachDatabase(db_path, "corrupt") ||
+          probe_db.GetErrorCode() != SQLITE_NOTADB) {
+        RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_BEGIN);
+        return;
+      }
+    }
+
+    // The database has invalid data in the SQLite header, so it is almost
+    // certainly not recoverable without manual intervention (and likely not
+    // recoverable _with_ manual intervention).  Clear away the broken database.
+    if (!sql::Connection::Delete(db_path)) {
+      RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NOTADB_DELETE);
+      return;
+    }
+
+    // Probe to see if that worked.  These reports should not be possible in a

[pwnall, 2017/02/27 22:28:33]

"that worked" -> "the database got deleted"?

(minor confusion regarding what's happening here)

[Scott Hess - ex-Googler, 2017/02/27 23:39:39]

Are you familiar with the problem where deleting a file doesn't actually work,
even though it claims to work?  Here's a super-complicated CL:
   https://codereview.chromium.org/2545283002/
The gist of it is that on Windows, file deletion doesn't really work like
everyone believes it works.  I've actually had cases in the past where
base::DeleteFile() returned true (meaning "deleted") but the file actually was
still there.

I think maybe it's better to just adjust the comment to not imply that this is
some new level of impossible versus the rest of the code.  That's just me
pre-judging the results, the histograms will provide real data.

[pwnall, 2017/02/27 23:54:51]

Sorry for being unclear! I understood the problem (and remember it... also,
AFAIK it bit the installer folks pretty hard in the past too). I didn't like the
vagueness of "that worked". I really just wanted more precise language, like
"Check to see if the file actually got deleted. " You could add something like
"On Windows, Delete() sometimes returns success, but the file stays behind." 

[Scott Hess - ex-Googler, 2017/02/28 01:14:51]

Done.

+    // properly-working system.
+    {
+      Connection probe_db;
+      if (!probe_db.Open(db_path)) {
+        RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NOTADB_REOPEN);
+        return;
+      }
+      if (!probe_db.Execute("PRAGMA auto_vacuum")) {
+        RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NOTADB_QUERY);
+        return;
+      }
+    }
+
+    // Recovery could be run on the re-opened database, but there is no point to
+    // that, as there is no schema or data to recover.
+    RecordRecoveryEvent(RECOVERY_SUCCESS_AUTORECOVERDB_NOTADB_DELETE);
     return;
   }