New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reject invalid wallets #10885
Reject invalid wallets #10885
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes for reviewers.
src/test/util_tests.cpp
Outdated
@@ -104,7 +104,7 @@ class TestArgsManager : public ArgsManager | |||
{ | |||
return mapArgs; | |||
}; | |||
const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs() | |||
std::map<std::string, std::vector<std::string> >& GetMapMultiArgs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a helper, like the above. For the test below I had to drop const
,
src/test/util_tests.cpp
Outdated
|
||
BOOST_CHECK_EQUAL(testArgs.GetUniqueArgs("a").size(), 0); | ||
|
||
testArgs.GetMapArgs()["a"] = "biz"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is set because IsArgSet
uses mapArgs
.
src/util.h
Outdated
@@ -202,6 +202,7 @@ class ArgsManager | |||
void ParseParameters(int argc, const char*const argv[]); | |||
void ReadConfigFile(const std::string& confPath); | |||
std::vector<std::string> GetArgs(const std::string& strArg); | |||
std::vector<std::string> GetUniqueArgs(const std::string& strArg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you briefly describe what it does in a comment above this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this use-case common enough to add a new argument getter to the API for?
If not, I'd prefer chaining more general functions, for example defining a function std::vector<string> UniqueStrings(const std::vector<string>&)
then using it as
UniqueStrings(GetArgs("-wallet"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other multiargs that may use this getter, but I'm checking it. There is also the option of GetArgs(const std::string& strArg, fUnique = false)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I also like that option @laanwj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to comment.
I can imagine this having wider use, so I don't think adding a ArgsManager member function is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
An easy way to add a functional test for this would be to rebase it on top of #10604 , add a duplicate wallet argument and then add a test that listwallets
returns the correct number of wallets. Check with @laanwj because we don't want to add that PR dependency if #10604 doesn't get into v0.15
src/util.cpp
Outdated
std::vector<std::string> ArgsManager::GetUniqueArgs(const std::string& strArg) | ||
{ | ||
LOCK(cs_args); | ||
std::vector<std::string> args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not let std::set do the heavy lifting for you:
std::vector<std::string> args = GetArgs(strArg);
std::set<std::string> uniques(args.begin(), args.end());
args.assign(uniques.begin(), uniques.end());
return args;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to keep order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah. Makes sense. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tempted to name it GetStableUniqueArgs
but maybe it's too much for most of you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just make sure there's a comment at the declaration saying that ordering is stable.
src/util.h
Outdated
@@ -202,6 +202,7 @@ class ArgsManager | |||
void ParseParameters(int argc, const char*const argv[]); | |||
void ReadConfigFile(const std::string& confPath); | |||
std::vector<std::string> GetArgs(const std::string& strArg); | |||
std::vector<std::string> GetUniqueArgs(const std::string& strArg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to comment.
I can imagine this having wider use, so I don't think adding a ArgsManager member function is a problem.
#10604 has been merged. Suggest
|
4b58cae
to
7605a8d
Compare
@jnewbery thanks for the tip, it's in 7605a8d. @instagibbs added the missing comment in 8cf06d7. |
src/util.h
Outdated
@@ -203,6 +203,10 @@ class ArgsManager | |||
void ReadConfigFile(const std::string& confPath); | |||
std::vector<std::string> GetArgs(const std::string& strArg); | |||
|
|||
// Returns unique values of the given argument, preserving the order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultranit: match the doxygen style comments below.
Looks good @promag . I've added one nitty comment. utACK either way. |
6385e70
to
1548982
Compare
@jnewbery added missing comment to |
please correct me if I'm wrong. As far as I can tell, since this only prevents specifying the same filename multiple times, it still doesn't prevent you from loading the same file twice. You just have to use different names/paths to refer to it:
One could also use hard- or soft links to give a single wallet multiple names. |
@arvidn I'll test. Thanks! |
src/util.cpp
Outdated
{ | ||
LOCK(cs_args); | ||
std::vector<std::string> args; | ||
std::set<std::string> uniques; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should #include <set>
I'm updating to take into account @arvidn review. |
1548982
to
b1b4e8a
Compare
After @arvidn observation, I took a different approach. In order to correctly detect duplicate wallets I'm using @jnewbery removed the functional test because the app now quits with an error, AFAIK we don't test these? |
@arvidn there is already a |
src/wallet/wallet.cpp
Outdated
return InitError(_("Invalid characters in -wallet filename")); | ||
} | ||
|
||
fs::path wallet_path = fs::absolute(walletFile, GetDataDir()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the documentation, I'm not sure that boost::filesystem::absolute
resolves symlinks; only that it resolves relative path elements (lack of root, and .
and ..
path elements). boost::filesystem::canonical
resolves symlinks too.
EDIT: nevermind, it seems that fs::equivalent
further takes care of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sipa fs::equivalent
is not used in the latest version.
utACK b1b4e8ac1687fac1dd275b7bab2c45eb879be30d |
src/wallet/wallet.cpp
Outdated
return InitError(_("Invalid characters in -wallet filename")); | ||
} | ||
|
||
fs::path wallet_path = fs::absolute(walletFile, GetDataDir()); | ||
|
||
if (fs::exists(wallet_path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this imply you can specify duplicate wallets if the wallet doesn't (yet) exist? It seems strange mostly because it would result in you being able to restart your node and then suddently it will fail but succeed the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7c4058d disallows having wallets as symlinks whereas addffb2 resolves symlink to the concrete absolute path.
This approach seems pretty fragile, but maybe it's the simplest safeguard we can implement right now. I'd think ideally bitcoin would try to acquire an exclusive write lock on a wallet database file while opening it, and just not open files that can't be locked. |
src/wallet/wallet.cpp
Outdated
return InitError(_("Invalid -wallet file")); | ||
} | ||
|
||
if (fs::is_symlink(wallet_path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this could be done using fs::canonical
?
Right, ideally it's something that would be handled at the database layer, in the same way it's avoided that multiple applications open the wallet at once. It's unfortunate that BerkeleyDB itself doesn't provide protection against opening a database multiple times. Anyhow it's better to have some check in place, so concept ACK. |
At this point there's no file opening, it's a configuration error. Like you said it's a runtime error. Both make sense for me. |
utACK. Would be nice to print the wallet which generated the error as @jnewbery suggested, but ready for merge either way. |
Yep - as Matt says, ready for merge with or without the error message improvement. Can always be added later. |
Suggested change to error messages here: https://github.com/jnewbery/bitcoin/tree/pr10885 . Feel free to cherry-pick or squash in. Or else I can open a PR after this is merged. |
Thanks @jnewbery, if you don't mind I'll squash. EDIT: Actually since you fixed the other error messages I'll leave it as is. |
fa11691
to
d84e78e
Compare
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
I know it's a bit late for this, but on upgrading to v0.15, this is a bit of a showstopper for me. I have the wallet.dat file as a symlink to another partition. The reasons for this are not entirely crazy:
Checking for the wallet being a symlink stops me from being able to do this, and I can't specify the full path on the -wallet command line argument as that only supports files in the datadir, not full paths. I don't think there's a neat solution to my problem, but I've discussed a few on #bitcoin-dev anyway.
I will live with option 3 if I need to, but was wondering if the symlink check is a bit of an overkill safety check here. Is it really likely the user will specify a -wallet argument for the real wallet, and another for a symlink to that real wallet? |
@bittylicious I understand this sounds like an unnecessary measure, but perhaps it is a bit more complicated than you realize. Effectively, the wallet is not just a single file, but a whole directory. Bitcoin Core will clean up all other files before shutdown, but in case of a crash, other files in the directory are needed for recovery. Symlinks may break these assumptions - making it look like a wallet file could be shared across datadirs, for example. I think the correct approach for the issue you have is an ability to specify a wallet datadir, separate from the node's datadir. |
@sipa Thanks for the response. I appreciate what you're saying and there's a reliance between the wallet and other files in the datadir. I think what I'm saying is that if you're messing around with symlinks, you're probably technical enough to know what you're doing, so having artificial restrictions like the symlink check probably won't result in much real safety. Having to find a workaround in this case is probably more risky. Re: wallet datadir, I'm not sure how that could work given dependencies between that and the regular datadir, or do you mean that it would essentially work as a flag to say "I really do know what I'm doing"? |
There are no dependencies between the wallet and the rest of the datadir, in either direction. |
Sorry, I meant wallet.dat dependencies (those that make up the full wallet during runtime). |
Having discussed this a bit more on #bitcoin-dev I see what you're saying - the wallet should really be in its own directory as a BDB database expects this, hence the suggestion for a -walletdir argument. |
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2
This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks.