From bc0090f1d60652ebbfd8fd1477b9b17cb9ade239 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 29 Dec 2025 14:23:24 -0500 Subject: [PATCH 1/3] wallet: handle non-writable db directories 1) For wallet load, this fixes a crash. We currently allow loading wallets located on non-writable directories. This is problematic because the node crashes on any subsequent write. E.g. generating a block is enough to trigger it. 2) For wallet creation, this improves the returned error msg. 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 : directory is not writable" --- src/util/fs_helpers.cpp | 27 ++++++++++++++++++++++++++- src/util/fs_helpers.h | 8 ++++++++ src/wallet/sqlite.cpp | 4 ++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index a41bf65ad86..097fbef9af3 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -6,8 +6,9 @@ #include // IWYU pragma: keep #include - +#include #include +#include #include // IWYU pragma: keep #include #include @@ -18,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -306,6 +308,29 @@ std::optional 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) { diff --git a/src/util/fs_helpers.h b/src/util/fs_helpers.h index face17fd8b5..f4d406f75f0 100644 --- a/src/util/fs_helpers.h +++ b/src/util/fs_helpers.h @@ -94,6 +94,14 @@ std::string PermsToSymbolicString(fs::perms p); */ std::optional 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 diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 3d6583bb037..17414521d0b 100644 --- a/src/wallet/sqlite.cpp +++ b/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))); From 0218966c0dceb8b9c25a26c92eb6902faa56fbd9 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 29 Dec 2025 14:34:09 -0500 Subject: [PATCH 2/3] test: add coverage for wallet creation in non-writable directory --- test/functional/test_framework/util.py | 10 ++++++++++ test/functional/wallet_createwallet.py | 21 +++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 8e1e3c716f6..3e7e6dd97a9 100644 --- a/test/functional/test_framework/util.py +++ b/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 diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py index 8edbbcf4b3f..bb7918a9a44 100755 --- a/test/functional/wallet_createwallet.py +++ b/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.", From 08925d5ee75c15712f523bacbdcfd44257f9c8ac Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 3 Jan 2026 12:37:45 -0500 Subject: [PATCH 3/3] test: add coverage for loading a wallet in a non-writable directory Previously, wallets in non-writable directories were loaded, leading to crashes on any subsequent write. --- test/functional/wallet_startup.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py index fe35ee12f9e..2c5fb259059 100755 --- a/test/functional/wallet_startup.py +++ b/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()