Ensure backupwallet fails when attempting to backup to source file

tomasvdw commented at 10:13 pm on September 20, 2017: contributor

Previous behaviour was to destroy the wallet (to zero-length)

This fixes #11375

fanquake added the label Wallet on Sep 20, 2017

esotericnonsense commented at 0:11 am on September 21, 2017: contributor

I like this.

Generalizing a bit, I don’t like the presence of ‘overwrite_if_exists’. It seems like a footgun opportunity in other cases (e.g. overwrite one wallet with another).

My preference would be to either make that impossible or gate it behind an rpc toggle (default being don’t overwrite, like when using ‘cp/mv –interactive’).

esotericnonsense commented at 0:26 am on September 21, 2017: contributor

Tested and working. Additional test performed (try to backup to a file that symlinks to the wallet)

 0$ ln -s ~/.bitcoin/regtest/wallet.dat ~/.bitcoin/regtest/wallet.dat.symlink
 1ln: failed to create symbolic link '/home/user/.bitcoin/regtest/wallet.dat.symlink': File exists
 2$ stat ~/.bitcoin/regtest/wallet.dat.symlink | head -n 1
 3  File: /home/user/.bitcoin/regtest/wallet.dat.symlink -> /home/user/.bitcoin/regtest/wallet.dat
 4$ ./bitcoin-cli -regtest backupwallet ~/.bitcoin/regtest/wallet.dat.symlink
 5error code: -4
 6error message:
 7Error: Wallet backup failed!
 8$ tail -n 1 ~/.bitcoin/regtest/debug.log
 92017-09-21 00:25:02 cannot backup to wallet source file /home/user/.bitcoin/regtest/wallet.dat.symlink
10$ stat ~/.bitcoin/regtest/wallet.dat | grep Size                                                                                                                    
11  Size: 1388544         Blocks: 2712       IO Block: 4096   regular file

gmaxwell commented at 1:51 am on September 21, 2017: contributor

Thanks. I think this cannot be guaranteed to work with hard links and whatnot, but it’s better than nothing.

in test/functional/walletbackup.py:194 in a673e6e946 outdated

189@@ -190,6 +190,22 @@ def run_test(self):
190         assert_equal(self.nodes[1].getbalance(), balance1)
191         assert_equal(self.nodes[2].getbalance(), balance2)
192 
193+        # backup to file self must fail
194+        try:

TheBlueMatt commented at 3:31 am on September 21, 2017:

You can replace these entire try:except: blocks with a simple assert_raises_jsonrpc call (which also checks the error number).

TheBlueMatt commented at 3:34 am on September 21, 2017: member

Code looks good, thanks! Just needs to clean up the test changes.

tomasvdw force-pushed on Sep 21, 2017

tomasvdw commented at 6:49 am on September 21, 2017: contributor

I’ve changed the test cases to use assert_raises_jsonrpc

sdaftuar commented at 3:22 pm on September 21, 2017: member

Yikes – thank you for reporting and fixing this bug.

I agree with @esotericnonsense and think we should go further; a command called “backupwallet” should never delete/overwrite anything, in my opinion.

in src/wallet/db.cpp:675 in e062d5883d outdated

671@@ -672,6 +672,12 @@ bool CWalletDBWrapper::Backup(const std::string& strDest)
672                     pathDest /= strFile;
673 
674                 try {
675+                    if (fs::equivalent(pathSrc, pathDest)) {

jnewbery commented at 3:41 pm on September 21, 2017:

Good that you’ve included this in the try-except block, since boost::filesystem::equivalent() can throw, but the generic catch behaviour below would hide the precise error. You could return a more precise error by:

using the equivalent(const path& p1, const path& p2, system::error_code& ec) form (http://www.boost.org/doc/libs/1_46_0/libs/filesystem/v3/doc/reference.html#equivalent) and checking the returned error_code
testing the error code in the catch block below
having two try-catch blocks

Perhaps a little bit overkill since the only way that boost::filesystem::equivalent() can throw is if the source wallet.dat file doesn’t exist or isn’t a regular file/directory/symlink. But something to consider.

tomasvdw commented at 5:05 pm on September 21, 2017:

but the generic catch behaviour below would hide the precise error.

It does dumps the error message

Perhaps a little bit overkill since the only way that boost::filesystem::equivalent() can throw is if the source wallet.dat file doesn’t exist or isn’t a regular file/directory/symlink.

This was also my thought. If equivalant() fails we have bigger problems and a generic error is sufficient.

in src/wallet/db.cpp:677 in e062d5883d outdated

671@@ -672,6 +672,12 @@ bool CWalletDBWrapper::Backup(const std::string& strDest)
672                     pathDest /= strFile;
673 
674                 try {
675+                    if (fs::equivalent(pathSrc, pathDest)) {
676+                        LogPrintf("cannot backup to wallet source file %s\n",
677+                            pathDest.string());

jnewbery commented at 3:41 pm on September 21, 2017:

nit: the repository doesn’t enforce max line length, so you can join this with the line above.

in test/functional/walletbackup.py:202 in e062d5883d outdated

197+            tmpdir + "/node0/regtest/",
198+            tmpdir + "/node0/regtest"]
199+
200+        for sourcePath in sourcePaths:
201+            assert_raises_jsonrpc(-4, "backup failed",
202+                    self.nodes[0].backupwallet, sourcePath)

jnewbery commented at 3:43 pm on September 21, 2017:

nit: again, no max line length, so you can join this with the line above.

jnewbery commented at 3:57 pm on September 21, 2017: member

Thanks for this @tomasvdw . This is really awful and dangerous behaviour so thanks for providing a fix so quickly. I’ve put a few comments inline, but some more general comments:

as others have already pointed out, fs::copy_option::overwrite_if_exists is completely inappropriate for a backup command. To my mind, a backup command should never result in data being deleted or overwritten (unless perhaps called with an explicit by-default-off overwrite option)
If a relative path is passed to backupwallet, then the backup will be to the directory relative to the directory where bitcoind was started. That’s confusing (imagine for example a user who has started bitcoind from the datadir, and then much later calls bitcoin-cli backupwallet ./). We should probably just reject relative paths entirely.
now that the backupwallet RPC can fail in more than one way, it’d be good to propagate the error message back up to the RPC function and return it to the user. That can be done by adding an std::string& error_string parameter to CWallet::BackupWallet() and CWalletDBWrapper::Backup(). Without this, the user would just see a generic “Error: Wallet backup failed!” message and not know that it was because he’d provided a path that matches the existing wallet.dat file.

Those points aside, this is a useful change as is. If you’re interested in implementing something that covers off those points, then I’d be very happy to review. Otherwise this can go in now and we can further improve behaviour in a future PR.

tomasvdw commented at 5:17 pm on September 21, 2017: contributor

@jnewbery , @sdaftuar Personally I think that silently overwriting the destination (if it’s not the source!) is perfectly reasonable for a backup api command, and expected behavior. You don’t want an rpc method to be interactive, and I am not to fond of something like “mode=overwrite”.

But if others agree with your point, I am happy to change this, in this PR.

With regards to the returning message, my reasoning is that although this is indeed quite a major bug it is also extremely rare and unlikely to happen to more than almost nobody. I considered a generic “fail” and a log enough, but if this reasoning is too lazy, I could enum the return and clarify the message. Do we want that?

With regards to relative paths; I would think that the RPC interface is primarily a programming interface. A relative path being relative to the working directory of the daemon seems to be common practice among programmable services, and removing it an odd limitation in flexibility that doesn’t really help anyone.

tomasvdw force-pushed on Sep 21, 2017

tomasvdw commented at 7:16 pm on September 21, 2017: contributor

Removed line breaks as per suggestion @jnewbery

sdaftuar commented at 7:43 pm on September 21, 2017: member

@tomasvdw I really think overwriting something that could be a wallet file a user needs is a fundamentally dangerous behavior; for example even though we’re checking that the destination is different from the source, now that we have multiwallet you could be overwriting some other wallet file (which might even be loaded, though that doesn’t really matter) – again resulting in funds loss.

I think the default behavior should not be to overwrite anything, and I’d be fine with making it so that backupwallet never overwrote anything (ie don’t bother with a option like mode=overwrite to allow a user to toggle it); it doesn’t seem too cumbersome to me to require users to move the backup out of the way first before overwriting, and it’s clearly safer.

Also it looks to me like boost::filesystem::copy_file is not doing an atomic copy/overwrite, so it’s possible that if your disk fills up in the middle or if your computer crashes, that the end result will be that you have now have no backup and that your existing backup has been deleted. I don’t think this is an outcome we should permit.

gmaxwell commented at 11:11 pm on September 21, 2017: contributor

as others have already pointed out, fs::copy_option::overwrite_if_exists is completely inappropriate for a backup command. To my mind, a backup command should never result in data being deleted or overwritten (unless perhaps called with an explicit by-default-off overwrite option)

The sort of standard way to use the dump command is to crontab running it to write to some place where your backup process is going to pick it up. If if doesn’t delete it after, you’re now backing up nothing. :(

I think it’s not helpful to complain about overwriting without having an answer to preventing this kind of silent backup failure.

Obvious alternatives off the top of my head are to halt and catch fire if you ask it to overwrite (so the above problem won’t last), or to move the file its overwriting out of the way first e.g. wallet.backup.dat.1

sdaftuar commented at 0:10 am on September 22, 2017: member

or to move the file its overwriting out of the way first e.g. wallet.backup.dat.1

Sure, this seems reasonable to me, as does having the rpc command return failure (which I’d expect someone running this out of cron to already need to be catching).

jonasschnelli added the label Needs backport on Sep 22, 2017

jonasschnelli commented at 5:11 am on September 22, 2017: contributor

utACK b688b5242714f64c36e237b2c5c4151164ee6eb3

jonasschnelli commented at 5:14 am on September 22, 2017: contributor

Added “Need Backport” label. Seems critical. This should catch most footgunish usecases and therefore is a clear improvement, but agree with @gmaxwell that it won’t catch symlink stupidity.

laanwj commented at 11:35 am on September 22, 2017: member

Concept ACK. I think it should refuse to overwrite any file, as @sdaftuar says.

I vaguely remember that we changed backupwallet a while ago to fail if it’s used on an existing file?

Edit: never mind - that was dumpwallet in #9937, and apparently it was never merged because people had different ideas about it.

jnewbery commented at 2:18 pm on September 22, 2017: member

I think it’s not helpful to complain without having an answer

I complained, and we’re now having a discussion, which is helpful in itself :slightly_smiling_face:

The ‘sort of standard way’ is not safe at all. Consider a setup where your cronjob is backing up to a filename, with a separate process or remote machine copying that backup file to a secure location. If that backup archiving process dies, then bitcoind will continue merrily overwriting old backups. Or imagine a scenario where you lose power and both your bitcoind process and backup archiving process die. You regain power and restart bitcoind, but perhaps set the wrong -mainnet/-testnet or -wallet parameter. You’ve now just blatted your most recent backup prior to the power outage.

(If I sound paranoid about this - good! I’ve supported too many customers who have got backups wrong and experienced a lot of pain later).

So at the very least I’d change your cronjob from:

bitcoin-cli backupwallet /path/to/backupdirectory/wallet_backup.dat

to:

bitcoin-cli backupwallet /path/to/backupdirectory/wallet_backup-$(date +%s).dat

or even better, to:

(bitcoin-cli backupwallet /path/to/backupdirectory/wallet_backup-$(date +%s).dat) || alert_or_stop_bitcoind.sh

So that’s the motivation. As for your potential solutions:

stop bitcoind if you ask it to overwrite (so the above problem won’t last): seems a bit extreme to me
move the file its overwriting out of the way first e.g. wallet.backup.dat.1. I think this is also unsafe. If there’s a file already at wallet.backup.dat, then there’s at least a chance that there’s another process that will modify or overwrite it later, or expect to find it there later. Imagine for example a slightly contrived example of trying to backup your wallet to the datadir of another running bitcoind. With this solution you’ve just changed the wallet.dat file from under the feet of the other bitcoind process, with bad consequences.

I think a better way would be to try to write the backup to the provided filename. If a file already exists, then try to write the backup to the provided filename with an index or timestamp suffix.

gmaxwell commented at 11:23 pm on September 25, 2017: contributor

If a file already exists, then try to write the backup to the provided filename with an index or timestamp suffix.

Why not move the existing file? The concern I have is that if the provided name is what your backup routine is backing up, you’ll just keep backing up the old thing over and over again.

Hm. I guess the move it out of the way doesn’t work so hot if the “thing in the way” is your live wallet.dat. :( can we just change the darn function to not let you set the filename, but only the path, and have it pick a file name, guaranteed to be unused, and return that?

tomasvdw commented at 7:05 am on September 26, 2017: contributor

I guess the move it out of the way doesn’t work so hot if the “thing in the way” is your live wallet.dat.

You could still leave in this fix (source != target). Generally, as there is some discussion on the move-out-of-way semantics, I think it is better to merge this critical fix and create a new PR to propose fixing overwrite. I think these are separate issues only related by touching the same area of code.

jnewbery commented at 5:25 pm on September 26, 2017: member

can we just change the darn function to not let you set the filename, but only the path, and have it pick a file name, guaranteed to be unused, and return that?

I’d be fine with that. Obviously it’s an API break.

I think my general approach is that the process that is backing itself up in general shouldn’t delete, overwrite or move files when backing up. It should be the backup archival process’s job to manage the backup files (polling for new files, copying to remote location, deleting or renaming the source backup file when done).

I think it is better to merge this critical fix and create a new PR to propose fixing overwrite

ACK. I think this is useful discussion, but I’m happy for this minimal fix to be merged and then to figure out a better more general approach in a different PR.

MarcoFalke added this to the milestone 0.15.1 on Oct 4, 2017

Ensure backupwallet fails when attempting to backup to source file

Previous behaviour was to destroy the wallet (to zero-length)

5d465e3962

in test/functional/walletbackup.py:201 in b688b52427 outdated

196+            tmpdir + "/node0/./regtest/wallet.dat",
197+            tmpdir + "/node0/regtest/",
198+            tmpdir + "/node0/regtest"]
199+
200+        for sourcePath in sourcePaths:
201+            assert_raises_jsonrpc(-4, "backup failed", self.nodes[0].backupwallet, sourcePath)

MarcoFalke commented at 7:08 pm on October 9, 2017:

Needs rebase and replace with “assert_raises_rpc_error”

tomasvdw commented at 12:51 pm on October 10, 2017:

Done; rebased and changed to assert_rpc_error

tomasvdw force-pushed on Oct 10, 2017

TheBlueMatt commented at 9:02 pm on October 17, 2017: member

Agreed, lets merge this and then we can paint the overwrite-parameter shed whatever color we like. utACK 5d465e396249a0e2cc60b16984a2bdbe4c8993c3

promag commented at 9:25 pm on October 17, 2017: member

If at the end the backup can’t overwrite any file then there is no need to merge this.

Edit: I mean, should be enough to check if the destination doesn’t exists to actually backup.

Edit 2: I agree with not allowing overwrites, some timestamp suffix/prefix is pretty common.

sdaftuar commented at 5:32 pm on October 31, 2017: member

Agree with @TheBlueMatt, though I do think we should never overwrite, better is better, let’s get this fix in for now.

utACK

in test/functional/walletbackup.py:194 in 5d465e3962

189@@ -190,6 +190,16 @@ def run_test(self):
190         assert_equal(self.nodes[1].getbalance(), balance1)
191         assert_equal(self.nodes[2].getbalance(), balance2)
192 
193+        # Backup to source wallet file must fail
194+        sourcePaths = [

MarcoFalke commented at 4:23 pm on November 1, 2017:

µnit: snake_case

MarcoFalke commented at 4:25 pm on November 1, 2017: member

utACK 5d465e396249a0e2cc60b16984a2bdbe4c8993c3. Seems a step in the right direction. No need let this rot again.

MarcoFalke merged this on Nov 1, 2017

MarcoFalke closed this on Nov 1, 2017

MarcoFalke referenced this in commit 1b8c88451b on Nov 1, 2017

MarcoFalke referenced this in commit eec251beeb on Nov 1, 2017

MarcoFalke removed the label Needs backport on Nov 1, 2017

MarcoFalke referenced this in commit fd79ed6b20 on Nov 1, 2017

codablock referenced this in commit e0b1792080 on Sep 26, 2019

codablock referenced this in commit b6116b20c0 on Sep 29, 2019

barrystyle referenced this in commit e42978fd46 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

Ensure backupwallet fails when attempting to backup to source file #11376