Bug 1942708 - sqlite3_close() must be invoked even if sqlite3_open_v2…

…() fails. r=asuth Differential Revision: https://phabricator.services.mozilla.com/D234999
mozilla · Jan 23, 2025 · 875863a · 875863a
1 parent 63e3181
commit 875863a
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 0 deletions.
diff --git a/storage/mozStorageConnection.cpp b/storage/mozStorageConnection.cpp
@@ -642,6 +642,8 @@ class AsyncBackupDatabaseFile final : public Runnable, public nsITimerCallback {
 
     int srv = ::sqlite3_open(NS_ConvertUTF16toUTF8(path).get(), &mBackupFile);
     if (srv != SQLITE_OK) {
+      ::sqlite3_close(mBackupFile);
+      mBackupFile = nullptr;
       return Dispatch(NS_ERROR_FAILURE, nullptr);
     }
 
@@ -1053,6 +1055,7 @@ nsresult Connection::initialize(const nsACString& aStorageKey,
   int srv = ::sqlite3_open_v2(path.get(), &mDBConn, mFlags,
                               basevfs::GetVFSName(true));
   if (srv != SQLITE_OK) {
+    ::sqlite3_close(mDBConn);
     mDBConn = nullptr;
     nsresult rv = convertResultCode(srv);
     RecordOpenStatus(rv);
@@ -1101,13 +1104,15 @@ nsresult Connection::initialize(nsIFile* aDatabaseFile) {
     srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn, mFlags,
                             basevfs::GetVFSName(exclusive));
     if (exclusive && (srv == SQLITE_LOCKED || srv == SQLITE_BUSY)) {
+      ::sqlite3_close(mDBConn);
       // Retry without trying to get an exclusive lock.
       exclusive = false;
       srv = ::sqlite3_open_v2(NS_ConvertUTF16toUTF8(path).get(), &mDBConn,
                               mFlags, basevfs::GetVFSName(false));
     }
   }
   if (srv != SQLITE_OK) {
+    ::sqlite3_close(mDBConn);
     mDBConn = nullptr;
     rv = convertResultCode(srv);
     RecordOpenStatus(rv);
@@ -1125,6 +1130,9 @@ nsresult Connection::initialize(nsIFile* aDatabaseFile) {
                             basevfs::GetVFSName(false));
     if (srv == SQLITE_OK) {
       rv = initializeInternal();
+    } else {
+      ::sqlite3_close(mDBConn);
+      mDBConn = nullptr;
     }
   }
 
@@ -1184,6 +1192,7 @@ nsresult Connection::initialize(nsIFileURL* aFileURL) {
 
   int srv = ::sqlite3_open_v2(spec.get(), &mDBConn, mFlags, vfs);
   if (srv != SQLITE_OK) {
+    ::sqlite3_close(mDBConn);
     mDBConn = nullptr;
     rv = convertResultCode(srv);
     RecordOpenStatus(rv);

diff --git a/storage/test/chrome/chrome.toml b/storage/test/chrome/chrome.toml
@@ -0,0 +1,3 @@
+[DEFAULT]
+
+["test_failedOpenConnection_leak.html"]
diff --git a/storage/test/chrome/test_failedOpenConnection_leak.html b/storage/test/chrome/test_failedOpenConnection_leak.html
@@ -0,0 +1,61 @@
+<!DOCTYPE HTML>
+<html>
+
+<head>
+  <meta charset="utf-8">
+  <title>Tests for the BackupSettings component</title>
+  <script src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <script src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
+  <link rel="stylesheet" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
+  <script>
+
+    const { Assert } = ChromeUtils.importESModule(
+      "resource://testing-common/Assert.sys.mjs"
+    );
+
+    /**
+     * Tests that failing to open a SQLite connection doesn't leak the SQLite
+     * structure. sqlite3_close() must always be invoked, even in case of failures.
+     * This will only fail on asan builds and requires shutdown leaks detection.
+     */
+
+    add_task(async function test_initWidget() {
+      let file = Services.dirsvc.get("ProfDS", Ci.nsIFile);
+      file.append("nonexisting.sqlite");
+
+      try {
+        file.remove(true);
+      } catch (ex) { }
+      await Assert.rejects(
+        new Promise((resolve, reject) => {
+          Services.storage.openAsyncDatabase(
+            file,
+            Ci.mozIStorageService.OPEN_READONLY,
+            Ci.mozIStorageService.CONNECTION_DEFAULT,
+            function (status, db) {
+              if (Components.isSuccessCode(status)) {
+                resolve(db.QueryInterface(Ci.mozIStorageAsyncConnection));
+              } else {
+                reject(new Components.Exception("Error opening database", status));
+              }
+            }
+          );
+        }),
+        /NS_ERROR_FILE_ACCESS_DENIED/,
+        "Should fail to open non existing database"
+      );
+    });
+
+    ok(true, "Must have at least one test");
+
+  </script>
+</head>
+
+<body>
+  <p id="display"></p>
+  <div id="content" style="display: none">
+  </div>
+  <pre id="test"></pre>
+</body>
+
+</html>
diff --git a/storage/test/moz.build b/storage/test/moz.build
@@ -5,6 +5,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 XPCSHELL_TESTS_MANIFESTS += ["unit/xpcshell.toml"]
+MOCHITEST_CHROME_MANIFESTS += ["chrome/chrome.toml"]
 
 TEST_DIRS += ["gtest"]
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		[DEFAULT]

		["test_failedOpenConnection_leak.html"]