util: if `DataDir` is a symbolic link, manually follow it #24267

pull nymkappa wants to merge 1 commits into bitcoin:master from nymkappa:bugfix/follow-symlinks changing 1 files +9 −0

nymkappa commented at 3:15 AM on February 5, 2022: none

Fixes #24257

$~/btc-things/bitcoin$ ln -s /media/DATA/Bitcoin /home/me/.bitcoin
$~/btc-things/bitcoin$ ./bitcoind 
************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: Not a directory [/home/me/.bitcoin]

Another related fix attempt can be found here https://github.com/bitcoin/bitcoin/pull/24266

If DataDir is a symbolic link, manually follow it 247a5c17c1
DrahtBot added the label Utils/log/libs on Feb 5, 2022
nymkappa marked this as ready for review on Feb 5, 2022
nymkappa marked this as a draft on Feb 5, 2022
DrahtBot commented at 8:58 AM on February 5, 2022: 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:
- #24274 (Introduce GetFileArg and use it where possible by prusnak)
- #24266 (util: Avoid buggy std::filesystem:::create_directories() call by hebasto)
- #24265 (Drop StripRedundantLastElementsOfPath() function by hebasto)
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.
in src/util/system.cpp:452 in 247a5c17c1
```
 447 | +    if (fs::is_symlink(path)) {
 448 | +        path = fs::read_symlink(path);
 449 | +        if (!fs::is_directory(path)) {
 450 | +            path = "";
 451 | +            return path;
 452 | +        }
```
hebasto commented at 9:07 AM on February 5, 2022:

What scenarios these lines are required in?

nymkappa commented at 9:08 AM on February 5, 2022:

A symbolic link may exist but point to a non existing location. For example, if your symbolic link point to an external hard drive, but it is not mounted.

nymkappa commented at 9:10 AM on February 5, 2022:

For example
nymkappa commented at 12:18 PM on February 5, 2022: none
I've tested the following scenarios. I run bitcoind from command line for each scenario and see what happens:
- Create a valid symlink with ln -s /home/me/Documents .bitcoin - OK (it read and writes into /home/me/Documents properly)
- Non existing ~/.bitcoin file/folder - OK (create a ~/.bitcoin folder)
- Existing ~/.bitcoin folder - OK (load the content of ~/.bitcoin folder)
- Create a broken symlink with ln -s /1234 .bitcoin - ??? (bitcoind does not start, but seems to crash, see below)
```
bitcoind: fs.cpp:39: fs::path fsbridge::AbsPathJoin(const fs::path&, const fs::path&): Assertion `base.is_absolute()' failed.
Aborted (core dumped)
```
- Create a new wallet using bitcoin-cli - OK (create ~/.bitcoin/wallets/xxx folder)
- Moved ~/.bitcoin/wallets/xxx to ~/.bitcoin/xxx - OK (I can load xxx wallet using bitcoin-cli)
nymkappa marked this as ready for review on Feb 5, 2022
nymkappa renamed this:
~~If DataDir is a symbolic link, manually follow it~~
util::If DataDir is a symbolic link, manually follow it
on Feb 5, 2022
nymkappa renamed this:
~~util::If DataDir is a symbolic link, manually follow it~~
util: if `DataDir` is a symbolic link, manually follow it
on Feb 5, 2022
hebasto commented at 4:47 PM on February 5, 2022: member

@nymkappa

I've tested the following scenarios...

If there are behavior changes comparing to pre-20744 variant, could describe them in the PR description?
nymkappa commented at 2:29 AM on February 6, 2022: none
@nymkappa

I've tested the following scenarios...

If there are behavior changes comparing to pre-20744 variant, could describe them in the PR description?

This is the only behavior change I've noticed from the previously mentioned scenario between master@04255073bbd2b8ea71ae8a9ff7433be499312758 (before #20744 was merged) and my pull request.

It looks like even though .bitcoin is a symbolic link (pointing to an invalid location), core does not try to follow it and instead tries to create a directory at that location.
- Create a broken symlink with ln -s /1234 .bitcoin - ??? (bitcoind does not start, but seems to crash, see below)
```
************************
EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::create_directory: File exists: "/home/me/.bitcoin"       
bitcoin in AppInit()       

bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.
Aborted (core dumped)
```
hebasto commented at 7:20 AM on February 6, 2022: member
This is the only behavior change I've noticed from the previously mentioned scenario between master@04255073bbd2b8ea71ae8a9ff7433be499312758 (before #20744 was merged) and my pull request.

FWIW, #24266 preserves the pre-20744 behavior, and for a broken link bitcoind reports an exception with a meaningful message:
```
$ src/bitcoind


************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: File exists [/home/hebasto/.bitcoin/wallets]       
bitcoin in AppInit()       
```
nymkappa commented at 8:10 AM on February 6, 2022: none
This is the only behavior change I've noticed from the previously mentioned scenario between master@04255073bbd2b8ea71ae8a9ff7433be499312758 (before #20744 was merged) and my pull request.

FWIW, #24266 preserves the pre-20744 behavior, and for a broken link bitcoind reports an exception with a meaningful message:

$ src/bitcoind ************************ EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE filesystem error: cannot create directories: File exists [/home/hebasto/.bitcoin/wallets] bitcoin in AppInit()
Closing this PR as #24266 provides a better fix. Thank you for your time hebasto 👍🏻
nymkappa closed this on Feb 6, 2022
DrahtBot locked this on Feb 6, 2023

Contributors

Labels

Linked (view graph)

#24257 bitcoind and bitcoin-cli does not supports symbolic link anymore #24266 util: Avoid buggy std::filesystem:::create_directories() call

util: if `DataDir` is a symbolic link, manually follow it #24267

Conflicts