[Database] Fails when opening a database in ReadOnly

This CL is fixing an corner case happening in the field while opening a
database.

When a Database is opened by an other process (example:
CopyFileEx(...)), sqlite may still open the database but in ReadOnly.
Any statement that attempt to modify the database will trigger an error.

This change will move the error code from SQLITE_IOERR_LOCK to
SQLITE_READONLY. The failure will be more determistic since it will fail
on open instead of on the execution of the first statement that attempt
to modify the database.

see:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/sqlite/src/src/os_win.c;l=5262;drc=bcf3df01928257644f91ead9a28b7b8487104508

Change-Id: I2046f03a8d16fc5b2754aa78a9a427a5c3df69c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6089240
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1407116}

net/extras/sqlite/sqlite_persistent_shared_dictionary_store_unittest.cc

@@ -1051,17 +1051,17 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
       SQLitePersistentSharedDictionaryStore::Error::kInvalidSql);
 }
 
-#if !BUILDFLAG(IS_FUCHSIA) && !BUILDFLAG(IS_WIN)
-// MakeFileUnwritable() doesn't cause the failure on Fuchsia and Windows. So
-// disabling the test on Fuchsia and Windows.
+#if !BUILDFLAG(IS_FUCHSIA)
+// MakeFileUnwritable() doesn't cause the failure on Fuchsia. So disabling the
+// test on Fuchsia.
 TEST_F(SQLitePersistentSharedDictionaryStoreTest,
        RegisterDictionaryErrorSqlExecutionFailure) {
   CreateStore();
   ClearAllDictionaries();
   DestroyStore();
   MakeFileUnwritable();
-  RunRegisterDictionaryFailureTest(
-      SQLitePersistentSharedDictionaryStore::Error::kFailedToExecuteSql);
+  RunRegisterDictionaryFailureTest(SQLitePersistentSharedDictionaryStore::
+                                       Error::kFailedToInitializeDatabase);
 }
 #endif  // !BUILDFLAG(IS_FUCHSIA)
 
@@ -1684,17 +1684,17 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
   CheckStoreRecovered();
 }
 
-#if !BUILDFLAG(IS_FUCHSIA) && !BUILDFLAG(IS_WIN)
-// MakeFileUnwritable() doesn't cause the failure on Fuchsia and Windows. So
-// disabling the test on Fuchsia and Windows.
+#if !BUILDFLAG(IS_FUCHSIA)
+// MakeFileUnwritable() doesn't cause the failure on Fuchsia. So disabling the
+// test on Fuchsia.
 TEST_F(SQLitePersistentSharedDictionaryStoreTest,
        ClearAllDictionariesErrorSqlExecutionFailure) {
   CreateStore();
   ClearAllDictionaries();
   DestroyStore();
   MakeFileUnwritable();
-  RunClearAllDictionariesFailureTest(
-      SQLitePersistentSharedDictionaryStore::Error::kFailedToSetTotalDictSize);
+  RunClearAllDictionariesFailureTest(SQLitePersistentSharedDictionaryStore::
+                                         Error::kFailedToInitializeDatabase);
 }
 #endif  // !BUILDFLAG(IS_FUCHSIA)
 
@@ -1742,18 +1742,18 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
       SQLitePersistentSharedDictionaryStore::Error::kInvalidSql);
 }
 
-#if !BUILDFLAG(IS_FUCHSIA) && !BUILDFLAG(IS_WIN)
-// MakeFileUnwritable() doesn't cause the failure on Fuchsia and Windows. So
-// disabling the test on Fuchsia and Windows.
+#if !BUILDFLAG(IS_FUCHSIA)
+// MakeFileUnwritable() doesn't cause the failure on Fuchsia. So disabling the
+// test on Fuchsia.
 TEST_F(SQLitePersistentSharedDictionaryStoreTest,
        ClearDictionariesErrorSqlExecutionFailure) {
   CreateStore();
   RegisterDictionary(isolation_key_, dictionary_info_);
   DestroyStore();
   MakeFileUnwritable();
-  RunClearDictionariesFailureTest(
-      base::RepeatingCallback<bool(const GURL&)>(),
-      SQLitePersistentSharedDictionaryStore::Error::kFailedToExecuteSql);
+  RunClearDictionariesFailureTest(base::RepeatingCallback<bool(const GURL&)>(),
+                                  SQLitePersistentSharedDictionaryStore::Error::
+                                      kFailedToInitializeDatabase);
 }
 
 TEST_F(SQLitePersistentSharedDictionaryStoreTest,
@@ -1764,7 +1764,8 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
   MakeFileUnwritable();
   RunClearDictionariesFailureTest(
       base::BindRepeating([](const GURL&) { return true; }),
-      SQLitePersistentSharedDictionaryStore::Error::kFailedToExecuteSql);
+      SQLitePersistentSharedDictionaryStore::Error::
+          kFailedToInitializeDatabase);
 }
 #endif  // !BUILDFLAG(IS_FUCHSIA)
 
@@ -1939,9 +1940,9 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
       SQLitePersistentSharedDictionaryStore::Error::kInvalidSql);
 }
 
-#if !BUILDFLAG(IS_FUCHSIA) && !BUILDFLAG(IS_WIN)
-// MakeFileUnwritable() doesn't cause the failure on Fuchsia and Windows. So
-// disabling the test on Fuchsia and Windows.
+#if !BUILDFLAG(IS_FUCHSIA)
+// MakeFileUnwritable() doesn't cause the failure on Fuchsia. So disabling the
+// test on Fuchsia.
 TEST_F(SQLitePersistentSharedDictionaryStoreTest,
        ProcessEvictionErrorSqlExecutionFailure) {
   CreateStore();
@@ -1949,8 +1950,8 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
   DestroyStore();
   MakeFileUnwritable();
 
-  RunProcessEvictionFailureTest(
-      SQLitePersistentSharedDictionaryStore::Error::kFailedToExecuteSql);
+  RunProcessEvictionFailureTest(SQLitePersistentSharedDictionaryStore::Error::
+                                    kFailedToInitializeDatabase);
 }
 #endif  // !BUILDFLAG(IS_FUCHSIA)
 
@@ -2074,9 +2075,9 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
                                     /*last_fetch_time=*/base::Time::Now()));
 }
 
-#if !BUILDFLAG(IS_FUCHSIA) && !BUILDFLAG(IS_WIN)
-// MakeFileUnwritable() doesn't cause the failure on Fuchsia and Windows. So
-// disabling the test on Fuchsia and Windows.
+#if !BUILDFLAG(IS_FUCHSIA)
+// MakeFileUnwritable() doesn't cause the failure on Fuchsia. So disabling the
+// test on Fuchsia.
 TEST_F(SQLitePersistentSharedDictionaryStoreTest,
        UpdateDictionaryLastFetchTimeErrorSqlExecutionFailure) {
   CreateStore();
@@ -2085,10 +2086,11 @@ TEST_F(SQLitePersistentSharedDictionaryStoreTest,
   DestroyStore();
   MakeFileUnwritable();
   CreateStore();
-  EXPECT_EQ(SQLitePersistentSharedDictionaryStore::Error::kFailedToExecuteSql,
-            UpdateDictionaryLastFetchTime(
-                register_dictionary_result.primary_key_in_database(),
-                /*last_fetch_time=*/base::Time::Now()));
+  EXPECT_EQ(
+      SQLitePersistentSharedDictionaryStore::Error::kFailedToInitializeDatabase,
+      UpdateDictionaryLastFetchTime(
+          register_dictionary_result.primary_key_in_database(),
+          /*last_fetch_time=*/base::Time::Now()));
 }
 #endif  // !BUILDFLAG(IS_FUCHSIA)
 

sql/database.cc

@@ -1984,9 +1984,25 @@ bool Database::OpenInternal(const std::string& db_file_path) {
     base::ElapsedTimer library_call_timer;
     sqlite_result_code = ToSqliteResultCode(sqlite3_open_v2(
         uri_file_path.c_str(), &db, open_flags, options_.vfs_name_discouraged));
+
+    // The database should not be opened in ReadOnly since the flag
+    // SQLITE_OPEN_READWRITE was specified. This condition is happening when the
+    // file can't be opened (already opened by an other process). This situation
+    // happens on a non-exclusive database when SQLite tries to re-open the file
+    // in read only after an initial failure. On Windows, the sqlite API
+    // fallback to open a database in read-only using flag SQLITE_OPEN_READONLY.
+    // The flag WINFILE_RDONLY will be added (see details within the sqlite
+    // function winOpen(...)). An error is reported here to avoid the following
+    // execute statements to fail to modify the database.
+    if (sqlite_result_code == SqliteResultCode::kOk && db &&
+        sqlite3_db_readonly(db, kSqliteMainDatabaseName) == 1) {
+      sqlite_result_code = SqliteResultCode::kReadOnly;
+    }
+
     RecordTimingHistogram("Sql.Database.Success.SqliteOpenTime.",
                           library_call_timer.Elapsed());
   }
+
   if (sqlite_result_code == SqliteResultCode::kOk) {
     db_ = db;
   } else {

sql/database_unittest.cc

@@ -2360,6 +2360,91 @@ TEST_P(SQLDatabaseTest, CheckpointDatabase) {
             "2");
 }
 
+#if BUILDFLAG(IS_WIN)
+
+TEST_P(SQLDatabaseTest, OpenFails_WindowsExclusiveReadMode) {
+  db_->Close();
+
+  base::File file(db_path_, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                                // Do not allow others to read from the file.
+                                base::File::FLAG_WIN_EXCLUSIVE_READ);
+  ASSERT_TRUE(file.IsValid());
+
+  sql::test::ScopedErrorExpecter expecter;
+  expecter.ExpectError(SQLITE_CANTOPEN);
+  ASSERT_FALSE(db_->Open(db_path_));
+  ASSERT_TRUE(expecter.SawExpectedErrors());
+  db_->Close();
+
+  file.Close();
+
+  ASSERT_TRUE(db_->Open(db_path_));
+}
+
+TEST_P(SQLDatabaseTest, OpenFails_WindowsExclusiveWriteMode) {
+  db_->Close();
+
+  base::File file(db_path_, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                                // Do not allow others to write to the file.
+                                base::File::FLAG_WIN_EXCLUSIVE_WRITE);
+  ASSERT_TRUE(file.IsValid());
+
+  sql::test::ScopedErrorExpecter expecter;
+  expecter.ExpectError(SQLITE_READONLY);
+  ASSERT_FALSE(db_->Open(db_path_));
+  ASSERT_TRUE(expecter.SawExpectedErrors());
+  db_->Close();
+
+  file.Close();
+
+  ASSERT_TRUE(db_->Open(db_path_));
+}
+
+TEST_P(SQLDatabaseTest, OpenFails_ExclusiveLock) {
+  db_->Close();
+
+  base::File file(db_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_EQ(base::File::FILE_OK, file.Lock(base::File::LockMode::kExclusive));
+
+  {
+    sql::test::ScopedErrorExpecter expecter;
+    expecter.ExpectError(SQLITE_IOERR_READ);
+    ASSERT_FALSE(db_->Open(db_path_));
+    ASSERT_TRUE(expecter.SawExpectedErrors());
+    db_->Close();
+  }
+
+  ASSERT_EQ(base::File::FILE_OK, file.Unlock());
+
+  ASSERT_TRUE(db_->Open(db_path_));
+}
+
+// This test is simulating an common error code received on Windows when
+// the database file is being copied by a third-party. The common API used
+// is CopyFileEx(...) which is acquiring a shared lock on the file.
+TEST_P(SQLDatabaseTest, OpenFails_SharedLock) {
+  db_->Close();
+
+  base::File file(db_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  ASSERT_TRUE(file.IsValid());
+  ASSERT_EQ(base::File::FILE_OK, file.Lock(base::File::LockMode::kShared));
+
+  {
+    sql::test::ScopedErrorExpecter expecter;
+    expecter.ExpectError(SQLITE_BUSY);
+    ASSERT_FALSE(db_->Open(db_path_));
+    ASSERT_TRUE(expecter.SawExpectedErrors());
+    db_->Close();
+  }
+
+  ASSERT_EQ(base::File::FILE_OK, file.Unlock());
+
+  ASSERT_TRUE(db_->Open(db_path_));
+}
+
+#endif  // BUILDFLAG(IS_WIN)
+
 TEST_P(SQLDatabaseTest, OpenFailsAfterCorruptSizeInHeader) {
   // The database file ends up empty if we don't create at least one table.
   ASSERT_TRUE(