Strip any trailing `/` in -datadir and -blocksdir paths #20080

hebasto · 2020-10-04T18:03:51Z

Wallet names in listwalletdir RPC are correct now, even if the -datadir path has any number of trailing /.

This PR is an alternative to #19933.

Fixes #19928.

src/util/system.cpp

hebasto · 2020-10-04T20:03:17Z

@promag As an author of #14561, mind looking into his PR?

Saibato · 2020-10-05T06:45:58Z

from the other PR #19933 ... to strip correctly,without boost > 1.47 siggh.

So if we could really now use the 1.58 that approach here concept tACK, since i might have choose this also if i was sure we had 1.58 and after the comment from @promag

TyI: i still find in the compile cache after make distclean
multiple lines like

/autom4te.cache/output.2:29476:  _AX_BOOST_BASE_TONUMERICVERSION_req="1.47.0"

But that's probably just trash form old version compiles?

hebasto · 2020-10-05T08:12:56Z

@Saibato

TyI: i still find in the compile cache after make distclean
multiple lines like
/autom4te.cache/output.2:29476:  _AX_BOOST_BASE_TONUMERICVERSION_req="1.47.0"  

You need something like git clean -fdx to clean the autom4te.cache directory.

But that's probably just trash form old version compiles?

Yes, it is a cache from your old compiles.

maflcko · 2020-10-05T13:04:53Z

review ACK de37a1d

hebasto · 2020-10-05T13:24:17Z

The utACK from @luke-jr is buried under "Show resolved":

lol, looks like #14146 was the culprit, released in v0.18.0.

Might as well just call it a bug in the dependency docs at this point I guess.

utACK

promag · 2020-10-05T17:42:15Z

ACK, Could add a test with trailing /?

maflcko · 2020-10-05T17:52:18Z

I wrote a test when I reviewed this, btw:

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 241c56934e..e9f2ba8e13 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -42,6 +42,17 @@ namespace BCLog {
 
 BOOST_FIXTURE_TEST_SUITE(util_tests, BasicTestingSetup)
 
+BOOST_AUTO_TEST_CASE(util_datadir)
+{
+    ClearDatadirCache();
+    const fs::path dd_norm = GetDataDir();
+    {
+        gArgs.ForceSetArg("-datadir", dd_norm.string() + "/");
+        ClearDatadirCache();
+        BOOST_CHECK_EQUAL(dd_norm, GetDataDir());
+    }
+}
+
 BOOST_AUTO_TEST_CASE(util_check)
 {
     // Check that Assert can forward

It fails on master:

test/util_tests.cpp(52): error: in "util_tests/util_datadir": check dd_norm == GetDataDir() has failed ["/tmp/test_common_Bitcoin Core/458e2f26ff0b2a2c4f007bdc719daaef01bf2da78133448fe78fddeb4f7f6c5d" != "/tmp/test_common_Bitcoin Core/458e2f26ff0b2a2c4f007bdc719daaef01bf2da78133448fe78fddeb4f7f6c5d/"]

Should pass on this branch.

hebasto · 2020-10-05T18:20:32Z

I wrote a test when I reviewed this, btw:

Thanks @MarcoFalke! Test added.

promag

Code review ACK b7c5a94.

maflcko · 2020-10-06T05:50:52Z

review ACK b7c5a94 💥

Show signature and timestamp

Signature:

Timestamp of file with hash ddb64044682ce642ee34bc19a33fc5a043fa5665c44db97a466c620be17f3743 -

src/test/util_tests.cpp

hebasto · 2020-10-07T08:55:37Z

Updated b7c5a94 -> 3cd0a16 (pr20080.02 -> pr20080.03):

addressed @promag's comment:

nit, could add another case lile + "/./" to it's clear the result is canonical, not just without last /.

rebased to work around CI: Appveyor builds failing #20066

meshcollider · 2020-10-07T09:18:52Z

Sorry I may be confused about the boost versions that this requires, but if this needs boost 1.58, it can't actually be backported to the branches tagged can it? This works on master but only the alternative #19933 could be backported?
EDIT: or does a boost version bump also apply to backport branches 🤔

hebasto · 2020-10-07T09:47:31Z

@meshcollider

Sorry I may be confused about the boost versions that this requires, but if this needs boost 1.58, it can't actually be backported to the branches tagged can it? This works on master but only the alternative #19933 could be backported?
EDIT: or does a boost version bump also apply to backport branches thinking

There is a discussion above.

ghost · 2020-10-11T20:51:34Z

Concept ACK

While I was testing few things with wallet names on Windows and Ubuntu, I found that user can create wallets with names:

../testwallet for Linux
..\testwallet for Windows

It creates the wallet in testnet3 instead of testnet3/wallets in the case of Ubuntu and testnet3\wallets for Windows which is kind of directory traversal although this is not a web application however used by lot of web applications so maybe we can add some code to validate user's input for a wallet name before saving.

hebasto · 2020-10-12T06:03:44Z

@prayank23

... maybe we can add some code to validate user's input for a wallet name before saving.

Good idea for a new pr.

maflcko · 2020-10-13T11:22:53Z

re-ACK 3cd0a16 , only change is adding even more tests 🦉

Show signature and timestamp

Signature:

Timestamp of file with hash 4e75eb321c4be66e5097c59196fb29d4285c9d8c96a0f6b0e3f80314a8e3612d -

hebasto · 2020-10-27T19:54:55Z

Updated 1c55140 -> ad5cef5 (pr20080.06 -> pr20080.07, diff):

addressed @MarcoFalke's comment

maflcko · 2020-10-28T12:59:17Z

review ACK ad5cef5 🔙

Show signature and timestamp

Signature:

Timestamp of file with hash 9a7e9ae08d0fbf3f86d95725fe0b1a1d791fdc2c42f0f77c5e92653f4a6141d2 -

promag

Code review ACK ad5cef5.

meshcollider

Code review + test run ACK ad5cef5

…r paths

Github-Pull: bitcoin#20080 Rebased-From: ad5cef5

fanquake · 2020-11-06T08:05:56Z

Added to #20166 for backporting to 0.20.

…oin#20080)

7566af4 doc: Update data directory path comments (Hennadii Stepanov) 09261de util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov) 8ef0dac macOS deploy: use the new plistlib API (Jonas Schnelli) 314e795 build: fix mutex detection when building bdb on macOS (fanquake) 1f67a30 random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman) 6113b54 net: Send post-verack handshake messages at most once (MarcoFalke) bdf15d0 rpc: Adjust witness-tx deserialize error message (MarcoFalke) 731502a rpc: Properly deserialize txs with witness before signing (MarcoFalke) ee0082b Avoid the use of abs64 in timedata (Pieter Wuille) 05bd0c2 docs: Correct description for getblockstats's txs field (Nadav Ivgi) Pull request description: Backports the following PRs to the 0.20 branch: * #19777 - docs: Correct description for getblockstats's txs field * #19836 - rpc: Properly deserialize txs with witness before signing * #20080 - Strip any trailing `/` in -datadir and -blocksdir paths * #20082 - [bugfix] random: fixes read buffer to use min rather than max * #20141 - Avoid the use of abs64 in timedata * #20146 - net: Send post-verack handshake messages at most once * #20195 - build: fix mutex detection when building bdb on macOS * #20298 - macOS deploy: use the new plistlib API Will add additional commits as they become available. ACKs for top commit: MarcoFalke: review ACK 7566af4 🗡 Tree-SHA512: add6bb978313c12c3e07bc232636ae9d1ab0edd0b816705c5c70eeb1cc04097165fd5e29d60c706886943ceb1f749a422020766b4aa2d23be51e9f839157a4bb

…oot directory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + Remove one character less from wallet path + After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR bitcoin/bitcoin#21907 helped me resolve the issue. **Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed **Solution**: `if` statement to check `walletdir` is a root directory Fixes: bitcoin/bitcoin#21510 bitcoin/bitcoin#21501 Related PR: bitcoin/bitcoin#20080 Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`: Before this PR: ``` { "wallets": [ { "name": "1" }, { "name": "2" } ] } ``` After this PR: ``` "wallets": [ { "name": "w1" }, { "name": "w2" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

…ctory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + Remove one character less from wallet path + After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR bitcoin#21907 helped me resolve the issue. **Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed **Solution**: `if` statement to check `walletdir` is a root directory Fixes: bitcoin#21510 bitcoin#21501 Related PR: bitcoin#20080 Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`: Before this PR: ``` { "wallets": [ { "name": "1" }, { "name": "2" } ] } ``` After this PR: ``` "wallets": [ { "name": "w1" }, { "name": "w2" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

Summary: This fixes a bug causing `listwalletdir` to strip lead char of wallet name is datadir has a trailing '/' Co-authored-by: saibato <saibato.naga@pm.me> Co-authored-by: MarcoFalke <falke.marco@gmail.com> This is a backport of [[bitcoin/bitcoin#20080 | core#20080]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10693

…ctory d44a261 Fix issues when `walletdir` is root directory (unknown) Pull request description: + Remove one character less from wallet path + After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR bitcoin#21907 helped me resolve the issue. **Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed **Solution**: `if` statement to check `walletdir` is a root directory Fixes: bitcoin#21510 bitcoin#21501 Related PR: bitcoin#20080 Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`: Before this PR: ``` { "wallets": [ { "name": "1" }, { "name": "2" } ] } ``` After this PR: ``` "wallets": [ { "name": "w1" }, { "name": "w2" } ] } ``` ACKs for top commit: ryanofsky: Code review ACK d44a261 meshcollider: utACK d44a261 Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55

This was referenced Oct 4, 2020

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

Closed

Sometimes wallet filenames can get their first character stripped off #19928

Closed

luke-jr suggested changes Oct 4, 2020

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

hebasto mentioned this pull request Oct 4, 2020

build: set minimum required Boost to 1.58.0 #19667

Merged

DrahtBot added the Utils/log/libs label Oct 4, 2020

promag reviewed Oct 5, 2020

View reviewed changes

maflcko added Needs backport (0.19) labels Oct 6, 2020

promag reviewed Oct 7, 2020

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 201004-strip branch from b7c5a94 to 3cd0a16 Compare October 7, 2020 08:50

ghost mentioned this pull request Oct 12, 2020

Validate user input and keep path in a separate argument for importwallet, createwallet and dumpwallet #20128

Closed

fanquake mentioned this pull request Oct 13, 2020

[0.20] build: set minimum required Boost to 1.48.0 #20142

Merged

Saibato mentioned this pull request Oct 13, 2020

Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks #19502

Merged

maflcko removed the Needs backport (0.18) label Oct 13, 2020

promag reviewed Oct 28, 2020

View reviewed changes

meshcollider reviewed Nov 1, 2020

View reviewed changes

meshcollider merged commit 5a6f3c5 into bitcoin:master Nov 1, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 2, 2020

Merge bitcoin#20080: Strip any trailing / in -datadir and -blocksdi…

4477249

…r paths

hebasto deleted the 201004-strip branch November 2, 2020 08:56

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Nov 6, 2020

doc: Update data directory path comments

7566af4

Github-Pull: bitcoin#20080 Rebased-From: ad5cef5

fanquake mentioned this pull request Nov 6, 2020

[0.20] Backports #20166

Merged

fanquake removed the Needs backport (0.20) label Nov 6, 2020

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 11, 2020

Strip any trailing in -datadir, -blocksdir and -paramsdir paths (bitc…

ed3cc9c

…oin#20080)

Saibato mentioned this pull request Dec 14, 2020

fix bitcoin issue #19928 also for knots bitcoinknots/bitcoin#28

Closed

ghost mentioned this pull request Dec 28, 2020

doc: Update 'Secure string handling' #20741

Merged

This was referenced Mar 22, 2021

BitCoinCore Wallet Naming #21501

Closed

Special characters in wallet_name #21510

Closed

ghost mentioned this pull request May 13, 2021

wallet: Fix issues when walletdir is root directory #21944

Merged

fanquake removed Needs backport (0.18) labels May 31, 2021

maflcko mentioned this pull request Oct 19, 2021

Use std::filesystem. Remove Boost Filesystem & System #20744

Merged

bitcoin locked and limited conversation to collaborators Jun 17, 2023

Strip any trailing / in -datadir and -blocksdir paths #20080

Strip any trailing / in -datadir and -blocksdir paths #20080

Uh oh!

Conversation

hebasto commented Oct 4, 2020

Uh oh!

Uh oh!

hebasto commented Oct 4, 2020

Uh oh!

Saibato commented Oct 5, 2020

Uh oh!

hebasto commented Oct 5, 2020

Uh oh!

maflcko commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 5, 2020

Uh oh!

promag commented Oct 5, 2020

Uh oh!

maflcko commented Oct 5, 2020

Uh oh!

hebasto commented Oct 5, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 6, 2020

Uh oh!

Uh oh!

hebasto commented Oct 7, 2020

Uh oh!

meshcollider commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 7, 2020

Uh oh!

ghost commented Oct 11, 2020

Uh oh!

hebasto commented Oct 12, 2020

Uh oh!

maflcko commented Oct 13, 2020

Uh oh!

hebasto commented Oct 27, 2020

Uh oh!

maflcko commented Oct 28, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 6, 2020

Uh oh!

Uh oh!

Strip any trailing `/` in -datadir and -blocksdir paths #20080

Strip any trailing `/` in -datadir and -blocksdir paths #20080

maflcko commented Oct 5, 2020 •

edited

Loading

meshcollider commented Oct 7, 2020 •

edited

Loading