Merge #14146: wallet: Remove trailing separators from -walletdir arg

2d471636eb9160ab51b08e491e3f003f57adbc36 wallet: Remove trailing separators from -walletdir arg (Pierre Rochard)
ea3009ee942188750480ca6cc273b2b91cf77ded wallet: Add walletdir arg unit tests (Pierre Rochard)

Pull request description:

  If a user passes in a path with a trailing separator as the `walletdir`, multiple BerkeleyEnvironments may be created in the same directory which can lead to data corruption.

  Discovered while reviewing https://github.com/bitcoin/bitcoin/pull/12493#issuecomment-417147646

Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09

5 files changed, 152 insertions, 3 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 6f3d29d652..4506d5dd6a 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -111,11 +111,14 @@ BITCOIN_TESTS += \
   wallet/test/psbt_wallet_tests.cpp \
   wallet/test/wallet_tests.cpp \
   wallet/test/wallet_crypto_tests.cpp \
-  wallet/test/coinselector_tests.cpp
+  wallet/test/coinselector_tests.cpp \
+  wallet/test/init_tests.cpp
 
 BITCOIN_TEST_SUITE += \
   wallet/test/wallet_test_fixture.cpp \
-  wallet/test/wallet_test_fixture.h
+  wallet/test/wallet_test_fixture.h \
+  wallet/test/init_test_fixture.cpp \
+  wallet/test/init_test_fixture.h
 endif
 
 test_test_bitcoin_SOURCES = $(BITCOIN_TEST_SUITE) $(BITCOIN_TESTS) $(JSON_TEST_FILES) $(RAW_TEST_FILES)
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index a299a4ee44..46983642f0 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -182,13 +182,18 @@ bool WalletInit::Verify() const
 
     if (gArgs.IsArgSet("-walletdir")) {
         fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
-        if (!fs::exists(wallet_dir)) {
+        boost::system::error_code error;
+        // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
+        fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error);
+        if (error || !fs::exists(wallet_dir)) {
             return InitError(strprintf(_("Specified -walletdir \"%s\" does not exist"), wallet_dir.string()));
         } else if (!fs::is_directory(wallet_dir)) {
             return InitError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), wallet_dir.string()));
+        // The canonical path transforms relative paths into absolute ones, so we check the non-canonical version
         } else if (!wallet_dir.is_absolute()) {
             return InitError(strprintf(_("Specified -walletdir \"%s\" is a relative path"), wallet_dir.string()));
         }
+        gArgs.ForceSetArg("-walletdir", canonical_wallet_dir.string());
     }
 
     LogPrintf("Using wallet directory %s\n", GetWalletDir().string());
diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp
new file mode 100644
index 0000000000..1453029c9c
--- /dev/null
+++ b/src/wallet/test/init_test_fixture.cpp
@@ -0,0 +1,42 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <fs.h>
+
+#include <wallet/test/init_test_fixture.h>
+
+InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName): BasicTestingSetup(chainName)
+{
+    std::string sep;
+    sep += fs::path::preferred_separator;
+
+    m_datadir = SetDataDir("tempdir");
+    m_cwd = fs::current_path();
+
+    m_walletdir_path_cases["default"] = m_datadir / "wallets";
+    m_walletdir_path_cases["custom"] = m_datadir / "my_wallets";
+    m_walletdir_path_cases["nonexistent"] = m_datadir / "path_does_not_exist";
+    m_walletdir_path_cases["file"] = m_datadir / "not_a_directory.dat";
+    m_walletdir_path_cases["trailing"] = m_datadir / "wallets" / sep;
+    m_walletdir_path_cases["trailing2"] = m_datadir / "wallets" / sep / sep;
+
+    fs::current_path(m_datadir);
+    m_walletdir_path_cases["relative"] = "wallets";
+
+    fs::create_directories(m_walletdir_path_cases["default"]);
+    fs::create_directories(m_walletdir_path_cases["custom"]);
+    fs::create_directories(m_walletdir_path_cases["relative"]);
+    std::ofstream f(m_walletdir_path_cases["file"].BOOST_FILESYSTEM_C_STR);
+    f.close();
+}
+
+InitWalletDirTestingSetup::~InitWalletDirTestingSetup()
+{
+    fs::current_path(m_cwd);
+}
+
+void InitWalletDirTestingSetup::SetWalletDir(const fs::path& walletdir_path)
+{
+    gArgs.ForceSetArg("-walletdir", walletdir_path.string());
+}
\ No newline at end of file
diff --git a/src/wallet/test/init_test_fixture.h b/src/wallet/test/init_test_fixture.h
new file mode 100644
index 0000000000..5684adbece
--- /dev/null
+++ b/src/wallet/test/init_test_fixture.h
@@ -0,0 +1,21 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_TEST_INIT_TEST_FIXTURE_H
+#define BITCOIN_WALLET_TEST_INIT_TEST_FIXTURE_H
+
+#include <test/test_bitcoin.h>
+
+
+struct InitWalletDirTestingSetup: public BasicTestingSetup {
+    explicit InitWalletDirTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
+    ~InitWalletDirTestingSetup();
+    void SetWalletDir(const fs::path& walletdir_path);
+
+    fs::path m_datadir;
+    fs::path m_cwd;
+    std::map<std::string, fs::path> m_walletdir_path_cases;
+};
+
+#endif // BITCOIN_WALLET_TEST_INIT_TEST_FIXTURE_H
diff --git a/src/wallet/test/init_tests.cpp b/src/wallet/test/init_tests.cpp
new file mode 100644
index 0000000000..7048547b2b
--- /dev/null
+++ b/src/wallet/test/init_tests.cpp
@@ -0,0 +1,78 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <boost/test/unit_test.hpp>
+
+#include <test/test_bitcoin.h>
+#include <wallet/test/init_test_fixture.h>
+
+#include <init.h>
+#include <walletinitinterface.h>
+#include <wallet/wallet.h>
+
+
+BOOST_FIXTURE_TEST_SUITE(init_tests, InitWalletDirTestingSetup)
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_default)
+{
+    SetWalletDir(m_walletdir_path_cases["default"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == true);
+    fs::path walletdir = gArgs.GetArg("-walletdir", "");
+    fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
+    BOOST_CHECK(walletdir == expected_path);
+}
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_custom)
+{
+    SetWalletDir(m_walletdir_path_cases["custom"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == true);
+    fs::path walletdir = gArgs.GetArg("-walletdir", "");
+    fs::path expected_path = fs::canonical(m_walletdir_path_cases["custom"]);
+    BOOST_CHECK(walletdir == expected_path);
+}
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_does_not_exist)
+{
+    SetWalletDir(m_walletdir_path_cases["nonexistent"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == false);
+}
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_is_not_directory)
+{
+    SetWalletDir(m_walletdir_path_cases["file"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == false);
+}
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_is_not_relative)
+{
+    SetWalletDir(m_walletdir_path_cases["relative"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == false);
+}
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
+{
+    SetWalletDir(m_walletdir_path_cases["trailing"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == true);
+    fs::path walletdir = gArgs.GetArg("-walletdir", "");
+    fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
+    BOOST_CHECK(walletdir == expected_path);
+}
+
+BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing2)
+{
+    SetWalletDir(m_walletdir_path_cases["trailing2"]);
+    bool result = g_wallet_init_interface.Verify();
+    BOOST_CHECK(result == true);
+    fs::path walletdir = gArgs.GetArg("-walletdir", "");
+    fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]);
+    BOOST_CHECK(walletdir == expected_path);
+}
+
+BOOST_AUTO_TEST_SUITE_END()