wallet: bugfix; if datadir has a trailing '/' listwalletdir would strip lead char of walletname

Saibato commented at 2:29 pm on September 10, 2020: contributor

fixes: #19928

DrahtBot added the label Wallet on Sep 10, 2020

luke-jr approved

luke-jr commented at 2:40 pm on September 10, 2020: member

Looks a bit fragile, but I guess better than nothing, and there’s a plan to replace this with a less-fragile version when Boost is bumped anyway. utACK

Saibato force-pushed on Sep 10, 2020

luke-jr changes_requested

luke-jr commented at 6:14 pm on September 10, 2020: member

Liked the first version better…

Saibato commented at 6:16 pm on September 10, 2020: contributor

Liked the first version better…

me 2 , but failed with some standard cases, i guess there are better way’s to do this? What we need is a patch without trailing’/’ siggh…

in src/wallet/walletutil.cpp:66 in 966907b529 outdated

62     std::vector<fs::path> paths;
63     boost::system::error_code ec;
64 
65+    // account for possible trailing backslash, since we have boost below 1,60
66+    while(wallet_dir.string()[offset-1] == '\\' || wallet_dir.string()[offset-1] == '/')
67+    {

jonatack commented at 6:23 pm on September 10, 2020:

Would it be safer to use .at() for bounds checks/no UB?

nit, clang-formatting

0-    while(wallet_dir.string()[offset-1] == '\\' || wallet_dir.string()[offset-1] == '/')
1-    {
2+    while (wallet_dir.string()[offset - 1] == '\\' || wallet_dir.string()[offset - 1] == '/') {

Saibato commented at 6:27 pm on September 10, 2020:

yup, draft ugly and no checks, maybe i close it, that PR is now like POC. What we need is an elegant way with just boost 1.47 to do this. Ideas? edit: what would be nice to have /> const fs::path path = fs::relative(it->path(), wallet_dir)

in src/wallet/walletutil.cpp:60 in 966907b529 outdated

56@@ -57,10 +57,16 @@ bool IsBerkeleyBtree(const fs::path& path)
57 std::vector<fs::path> ListWalletDir()
58 {
59     const fs::path wallet_dir = GetWalletDir();
60-    const size_t offset = wallet_dir.string().size() + 1;
61+    size_t offset = wallet_dir.string().size();

luke-jr commented at 6:44 pm on September 10, 2020:

Maybe try (wallet_dir / "dummy").remove_filename().string().size()

in src/wallet/walletutil.cpp:77 in 966907b529 outdated

74@@ -69,7 +75,7 @@ std::vector<fs::path> ListWalletDir()
75 
76         // Get wallet path relative to walletdir by removing walletdir from the wallet path.
77         // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
78-        const fs::path path = it->path().string().substr(offset);
79+        const fs::path path = it->path().string().substr(offset + 1);

luke-jr commented at 6:46 pm on September 10, 2020:

Suggest we assert substr(0, offset) == what we used for calculating offset above

Saibato force-pushed on Sep 10, 2020

wallet: bugfix; if datadir has a trailing '/' we would strip leading char of the walletname

If we select the path relative we strip the trailing '/'.

Signed-off-by: saibato <saibato.naga@pm.me>

7be117c585

Saibato force-pushed on Sep 11, 2020

promag commented at 10:54 am on September 11, 2020: member

Could GetWalletDir() return a “clean” path?

Saibato commented at 11:36 am on September 11, 2020: contributor

Could GetWalletDir() return a “clean” path?

That would be nice to have. It defaults in the error case we tackle here to GetDataDir() we would need then in GetWalletDir() to strip all trailing chars off type fs::path::preferred_separator. there. But that could also tangle deep into other parts where we need GetWalletDir.

Tested that a bit, but then I found that path could be also like “/tmp////…\.////test…///\/////” a hell to strip correctly,without boost > 1.47 siggh. To go for size was so far what worked with all sorts of path and syminks. And here we only build somewhat symlinks to the real wallet names an path .

At least it will return at minimum a single “.” so it will not empty for the back() string call.

edit@saibato @promag this alternative patch just in GetWalletDir() would also work to mitigate the error. like suggest. Not sure whats better and if there no side effects to other parts? So far i counted 7 references.

At least some fewer lines so less error?

 0diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp
 1index e4c72aed9..afd915f82 100644
 2--- a/src/wallet/walletutil.cpp
 3+++ b/src/wallet/walletutil.cpp
 4@@ -20,6 +20,9 @@ fs::path GetWalletDir()
 5        }
 6    } else {
 7        path = GetDataDir();
 8+        if (path.string().back() == fs::path::preferred_separator) {
 9+            path = path.parent_path().string();
10+        }
11        // If a wallets directory exists, use that, otherwise default to GetDataDir
12        if (fs::is_directory(path / "wallets")) {
13            path /= "wallets";

Saibato marked this as ready for review on Sep 11, 2020

tryphe commented at 5:25 am on September 16, 2020: contributor

Concept ACK. Prefer the patch above rather than the active PR. Logic is easier on the eyes and we need it everytime we call GetWalletDir(). As promag mentioned, it would be nice to know why the underlying function differs between systems, rather than adding this logic on. If it’s necessary, I think this is a good solution, but if other path functions need this logic, would prefer something like a GetCleanPath static function.

luke-jr commented at 12:17 pm on September 16, 2020: member

My “obsolete” comments above remain unaddressed. Would like a test to reproduce it if possible - I have been unable to do so.

Saibato commented at 7:22 pm on September 16, 2020: contributor

@tryphe

GetCleanPath static function.

hmm,,,, good idea, seams reasonable.

Btw; where u also able to reproduce the bug with current master i.e. with cc @luke-jr

Would like a test to reproduce it if possible

 0mkdir /tmp/wallet_test
 1bitcoind -daemon -datadir=/tmp/wallet_test -connect=127.0.0.1
 2sleep 2
 3bitcoin-cli -datadir=/tmp/wallet_test createwallet bitcoin1 
 4sleep 2
 5bitcoin-cli  -datadir=/tmp/wallet_test createwallet bitcoin2 
 6sleep 2
 7bitcoin-cli  -datadir=/tmp/wallet_test stop 
 8sleep 2
 9bitcoind -daemon -datadir=/tmp/wallet_test/ -connect=127.0.0.1 
10sleep 2
11bitcoin-cli  -datadir=/tmp/wallet_test listwalletdir
12sleep 2
13bitcoin-cli  -datadir=/tmp/wallet_test stop

Output should look like

 0{
 1  "wallets": [
 2    {
 3      "name": "itcoin2"
 4    },
 5    {
 6      "name": "itcoin1"
 7    },
 8    {
 9      "name": ""
10    }
11  ]
12}

if u open it the next time with i.e. bitcoind -daemon -datadir=/tmp/wallet_test/// even more chars are missing.

DrahtBot commented at 1:41 pm on September 19, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18095 (Fix crashes and infinite loop in ListWalletDir() by uhliksk)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke added this to the milestone 0.21.0 on Sep 19, 2020

Saibato commented at 4:57 pm on September 21, 2020: contributor

TYI: This behavior is AFAICS only with mainnet nodes, what makes it even more annoying since u can not test it easy before it hits u in cli or qt gui with real coins.

Might be not the right place here, but when I see 450Mil inflow in something like this with bold speech; i ask myself are they serious not to fund before that 1000 devs that feel a duty to lock at shit like this ( this is core not a an alt)?

Not that this here is at first glance a big deal ( but who knows what might be else there?)

I ask myself have they any clue or time to test and are there really no devs and users out there that can just confirm this easy test bash and clickbait this PR or do anything else but let it not happen that this kind of untested footguns are out there and not get backported fast.

This kind of shit can in my view really scare ppl away from core.

If i would see my precious wallets get fancy names behind my back in the reference implementation only if I just do one typo, that btw is not even wrong but an allowed way to type dir names. I would freak out.

Knowing that some of the reference code is probably copy pasted to a ton of of alts and other app’s . Rant stop here. but hey this PR sits 11 days here,

I know TAPROOT and Wall Street happiness and doxing Tor shit has prio, but hey if boldness top vetted super reviewed best coin talk and reality differ that much, i wonder wonder wonder.

TYI: I do this in my spare time ( ok, i have lot ) and not with a 100 devs team but since i locked in this some month ago i already had found 7 mayor things that where utterly wrong.

So pls give me a hint that this is work in progess and not only https://github.com/bitcoin/bitcoin/milestone/45

MarcoFalke commented at 7:14 pm on September 22, 2020: member

This kind of shit can in my view really scare ppl away from core.

The bug only exists in the master branch, which is known to have bugs and should not be used in production. It is good that this issue was found. There won’t be a 0.21 release without this bug fixed, which is why I assigned that milestone. You can find the schedule for the release here: #18947

Saibato commented at 6:54 pm on September 23, 2020: contributor

Thx @MarcoFalke but tyi, the bug exits in every release and some in alt projects that followed core since we have multiple wallets 18,x,19.x,,20.x, I build for test from source to compare.

MarcoFalke removed this from the milestone 0.21.0 on Sep 23, 2020

MarcoFalke added the label Needs backport (0.19) on Sep 23, 2020

MarcoFalke added the label Needs backport (0.20) on Sep 23, 2020

MarcoFalke added this to the milestone 0.20.2 on Sep 23, 2020

meshcollider commented at 11:09 am on September 29, 2020: contributor

Tested ACK 7be117c58537d97db19a5f6f9ea268ac6c8ec49f

It seems the problem only appears using mainnet as mentioned.

This seems to be because only the trailing separators cause it (doesn’t matter if there are multiple separators mid-path), and when using regtest or testnet, the path is automatically appended with the appropriate sub-directory, and no slash. Thus another fix would just be to always append /. to the path. Not sure which hack is uglier.

in src/wallet/walletutil.cpp:63 in 7be117c585

56@@ -57,10 +57,12 @@ bool IsBerkeleyBtree(const fs::path& path)
57 std::vector<fs::path> ListWalletDir()
58 {
59     const fs::path wallet_dir = GetWalletDir();
60-    const size_t offset = wallet_dir.string().size() + 1;
61     std::vector<fs::path> paths;
62     boost::system::error_code ec;
63 
64+    // account for possible trailing backslash, since we might have only boost 1.47 we could not use fs::relative

hebasto commented at 10:55 am on October 4, 2020:

Now we have boost 1.58 (#19667).

luke-jr commented at 3:25 pm on October 4, 2020:

Not in supported stable versions.

Saibato commented at 3:42 pm on October 4, 2020:

I guessed it might be backported at least to 0.19 and in 0.18 the bug is only int qt gui so 1.47 might be ok

But In hindsight and since u both look at this, the comment at line 73 is not changed by the PR and looks to me like a lingering Déjà vu with the GCC bug? Should we change or remove that, so that later no one grabs that trap when we change to std fs::? line 73: // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.

hebasto commented at 3:48 pm on October 4, 2020:

I think backporting pulls should consider boost 1.47, not this one.

MarcoFalke commented at 3:50 pm on October 4, 2020:

It might be better if the comment said what version is requried, as opposed to the version that we have right now

Saibato commented at 4:22 pm on October 4, 2020:

0    // account for possible trailing backslash,  if we would have bumped to boost 1.60 we could use a simple  fs::relative call

hebasto changes_requested

hebasto commented at 10:56 am on October 4, 2020: member

Concept ACK.

hebasto commented at 6:04 pm on October 4, 2020: member

Mind looking into alternative #20080?

Saibato commented at 7:46 pm on October 4, 2020: contributor

Mind looking into alternativ @hebasto tested #20080 and does pass my simple test bash and in the gui . not sure that fs::canonical is correct for all other instances and all imaginable path and symlinks i can think of where we use getdatadir.so for now i leave this here open as it is boost and make and backwards compatible,

But i also think that stripping in general the ‘/’ is probably the better way but a great mess to test all implications of that. So might it then be not better to not start the server at all if one try’s to use such paths with trailing ‘/’.

Also if u take charge at this i am fine with this since its probably by its implications more a thing for current core devs than for me.

edit@saibato tyi; this here strips also any number of ‘/’

meshcollider commented at 7:16 pm on October 15, 2020: contributor

Saibato, really appreciate you following through with this PR and contributing a fix! I’m sorry to say though that it appears the consensus is in favour of the approach in #20080 over this one, so I’m going to have to close this. But I appreciate your effort :tada:

meshcollider closed this on Oct 15, 2020

meshcollider removed this from the milestone 0.20.2 on Oct 15, 2020

MarcoFalke removed the label Needs backport (0.19) on Oct 15, 2020

MarcoFalke removed the label Needs backport (0.20) on Oct 15, 2020

hebasto commented at 9:21 am on October 16, 2020: member

@meshcollider @Saibato #20080 (comment)

meshcollider referenced this in commit 5a6f3c5a01 on Nov 1, 2020

DrahtBot locked this on Dec 25, 2022

wallet: bugfix; if datadir has a trailing ‘/’ listwalletdir would strip lead char of walletname #19933

Conflicts