Merge bitcoin/bitcoin#34176: wallet: crash fix, handle non-writable db directories

08925d5ee7 test: add coverage for loading a wallet in a non-writable directory (furszy)
0218966c0d test: add coverage for wallet creation in non-writable directory (furszy)
bc0090f1d6 wallet: handle non-writable db directories (furszy)

Pull request description:

  Make wallet creation and load fail with a clear error when the db directory isn’t writable.

  #### 1) For Wallet Creation

  Before: creating a wallet would return a generic error:
  "SQLiteDatabase: Failed to open database: unable to open database file"

  After: creating a wallet returns:
  "SQLiteDatabase: Failed to open database in directory <dir_path>: directory is not writable"

  #### 2) For Wallet Loading

  We currently allow loading wallets located on non-writable directories. This is problematic
  because the node crashes on any subsequent write; generating a block is enough to trigger it.
  Can be verified just by running the following test on master: 85fa4e2910

  Also, to check directory writability, this creates a tmp file rather than relying on the
  `permissions()` functions, since perms bits alone may not reliably reflect actual writability
  in some systems.

  Testing Note:
  Pushed the tests in separate commits so they can be cherry-picked on master for comparison.

ACKs for top commit:
  rkrux:
    re-ACK 08925d5ee7
  achow101:
    ACK 08925d5ee7
  seduless:
    Tested ACK 08925d5ee7

Tree-SHA512: e480eab329a1d595fe0b191e83c97956e3ff1d1e335ada8ac6fe72bc4b2bb9b13b0d49db0254d34ad75f816db06d9cd0c21d3063d7d8ee6687a7ea2324c36288

src/util/fs_helpers.cpp

@@ -6,8 +6,9 @@
 #include <bitcoin-build-config.h> // IWYU pragma: keep
 
 #include <util/fs_helpers.h>
-
+#include <random.h>
 #include <sync.h>
+#include <tinyformat.h>
 #include <util/byte_units.h> // IWYU pragma: keep
 #include <util/fs.h>
 #include <util/log.h>
@@ -18,6 +19,7 @@
 #include <map>
 #include <memory>
 #include <optional>
+#include <stdexcept>
 #include <string>
 #include <system_error>
 #include <utility>
@@ -306,6 +308,29 @@ std::optional<fs::perms> InterpretPermString(const std::string& s)
     }
 }
 
+bool IsDirWritable(const fs::path& dir_path)
+{
+    // Attempt to create a tmp file in the directory
+    if (!fs::is_directory(dir_path)) throw std::runtime_error(strprintf("Path %s is not a directory", fs::PathToString(dir_path)));
+    FastRandomContext rng;
+    const auto tmp = dir_path / fs::PathFromString(strprintf(".tmp_%d", rng.rand64()));
+
+    const char* mode;
+#ifdef __MINGW64__
+    mode = "w"; // Temporary workaround for https://github.com/bitcoin/bitcoin/issues/30210
+#else
+    mode = "wx";
+#endif
+
+    if (const auto created{fsbridge::fopen(tmp, mode)}) {
+        std::fclose(created);
+        std::error_code ec;
+        fs::remove(tmp, ec); // clean up, ignore errors
+        return true;
+    }
+    return false;
+}
+
 #ifdef __APPLE__
 FSType GetFilesystemType(const fs::path& path)
 {

src/util/fs_helpers.h

@@ -94,6 +94,14 @@ std::string PermsToSymbolicString(fs::perms p);
  */
 std::optional<fs::perms> InterpretPermString(const std::string& s);
 
+/** Check if a directory is writable by creating a temporary file on it.
+ *
+ * @param[in] dir_path Path of the directory to test
+ * @return true if a temporary file could be created and removed, false otherwise.
+ * @throw std::runtime_error if dir_path is not a directory.
+ */
+bool IsDirWritable(const fs::path& dir_path);
+
 #ifdef WIN32
 fs::path GetSpecialFolderPath(int nFolder, bool fCreate = true);
 #endif

src/wallet/sqlite.cpp

@@ -257,7 +257,11 @@ void SQLiteDatabase::Open(int additional_flags)
     if (m_db == nullptr) {
         if (!(flags & SQLITE_OPEN_MEMORY)) {
             TryCreateDirectories(m_dir_path);
+            if (!IsDirWritable(m_dir_path)) {
+                throw std::runtime_error(strprintf("SQLiteDatabase: Failed to open database in directory '%s': directory is not writable", fs::PathToString(m_dir_path)));
+            }
         }
+
         int ret = sqlite3_open_v2(m_file_path.c_str(), &m_db, flags, nullptr);
         if (ret != SQLITE_OK) {
             throw std::runtime_error(strprintf("SQLiteDatabase: Failed to open database: %s\n", sqlite3_errstr(ret)));

test/functional/test_framework/util.py

@@ -749,3 +749,13 @@ def wallet_importprivkey(wallet_rpc, privkey, timestamp, *, label=""):
     }]
     import_res = wallet_rpc.importdescriptors(req)
     assert_equal(import_res[0]["success"], True)
+
+def is_dir_writable(dir_path: pathlib.Path) -> bool:
+    """Return True if we can create a file in the directory, False otherwise"""
+    try:
+        tmp = dir_path / f".tmp_{random.randrange(1 << 32)}"
+        tmp.touch()
+        tmp.unlink()
+        return True
+    except OSError:
+        return False

test/functional/wallet_createwallet.py

@@ -4,12 +4,15 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test createwallet arguments.
 """
+import os
+import stat
 
 from test_framework.descriptors import descsum_create
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
+    is_dir_writable,
     wallet_importprivkey,
 )
 from test_framework.wallet_util import generate_keypair, WalletUnlock
@@ -25,9 +28,27 @@ class CreateWalletTest(BitcoinTestFramework):
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
+    def test_bad_dir_permissions(self, node):
+        self.log.info("Test wallet creation failure due to non-writable directory")
+        wallet_name = "bad_permissions"
+        dir_path = node.wallets_path / wallet_name
+        dir_path.mkdir(parents=True)
+        original_dir_perms = dir_path.stat().st_mode
+        os.chmod(dir_path, original_dir_perms & ~(stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH))
+        if is_dir_writable(dir_path):
+            self.log.warning("Skipping non-writable directory test: unable to enforce read-only permissions")
+        else:
+            # Run actual test
+            assert_raises_rpc_error(-4, f"SQLiteDatabase: Failed to open database in directory '{str(dir_path)}': directory is not writable", node.createwallet, wallet_name=wallet_name)
+        # Reset directory permissions for cleanup
+        dir_path.chmod(original_dir_perms)
+
+
     def run_test(self):
         node = self.nodes[0]
 
+        self.test_bad_dir_permissions(node)
+
         self.log.info("Run createwallet with invalid parameters.")
         # Run createwallet with invalid parameters. This must not prevent a new wallet with the same name from being created with the correct parameters.
         assert_raises_rpc_error(-4, "Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.",

test/functional/wallet_startup.py

@@ -6,12 +6,17 @@
 
 Verify that a bitcoind node can maintain list of wallets loading on startup
 """
+import os
 import shutil
+import stat
 import uuid
 
+
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_raises_rpc_error,
+    is_dir_writable,
 )
 
 
@@ -38,6 +43,27 @@ class WalletStartupTest(BitcoinTestFramework):
         shutil.move(self.nodes[0].wallets_path / wallet_name / "wallet.dat", self.nodes[0].wallets_path / "wallet.dat")
         (self.nodes[0].wallets_path / wallet_name).rmdir()
 
+    def test_load_unwritable_wallet(self, node):
+        self.log.info("Test wallet load failure due to non-writable directory")
+        wallet_name = "bad_permissions"
+
+        node.createwallet(wallet_name)
+        node.unloadwallet(wallet_name)
+
+        dir_path = node.wallets_path / wallet_name
+        original_dir_perms = dir_path.stat().st_mode
+        os.chmod(dir_path, original_dir_perms & ~(stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH))
+
+        if is_dir_writable(dir_path):
+            self.log.warning("Skipping load non-writable directory test: unable to enforce read-only permissions")
+        else:
+            # Ensure we don't load a wallet located in a non-writable directory.
+            # The node will crash later on if we cannot write to disk.
+            assert_raises_rpc_error(-4, f"SQLiteDatabase: Failed to open database in directory '{str(dir_path)}': directory is not writable", node.loadwallet, wallet_name)
+
+        # Reset directory permissions for cleanup
+        dir_path.chmod(original_dir_perms)
+
     def run_test(self):
         self.log.info('Should start without any wallets')
         assert_equal(self.nodes[0].listwallets(), [])
@@ -67,5 +93,7 @@ class WalletStartupTest(BitcoinTestFramework):
         self.restart_node(0)
         assert_equal(set(self.nodes[0].listwallets()), set(('w2', 'w3')))
 
+        self.test_load_unwritable_wallet(self.nodes[0])
+
 if __name__ == '__main__':
     WalletStartupTest(__file__).main()