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

tomasvdw · 2017-09-20T22:13:50Z

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

This fixes #11375

esotericnonsense · 2017-09-21T00:11:03Z

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 · 2017-09-21T00:26:56Z

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

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

gmaxwell · 2017-09-21T01:51:59Z

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

TheBlueMatt

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

TheBlueMatt · 2017-09-21T03:31:42Z

test/functional/walletbackup.py

@@ -190,6 +190,22 @@ def run_test(self):
        assert_equal(self.nodes[1].getbalance(), balance1)
        assert_equal(self.nodes[2].getbalance(), balance2)

+        # backup to file self must fail
+        try:


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

tomasvdw · 2017-09-21T06:49:37Z

I've changed the test cases to use assert_raises_jsonrpc

sdaftuar · 2017-09-21T15:22:41Z

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.

jnewbery

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.

jnewbery · 2017-09-21T15:41:26Z

src/wallet/db.cpp

@@ -672,6 +672,12 @@ bool CWalletDBWrapper::Backup(const std::string& strDest)
                    pathDest /= strFile;

                try {
+                    if (fs::equivalent(pathSrc, pathDest)) {


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.

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.

jnewbery · 2017-09-21T15:41:49Z

src/wallet/db.cpp

@@ -672,6 +672,12 @@ bool CWalletDBWrapper::Backup(const std::string& strDest)
                    pathDest /= strFile;

                try {
+                    if (fs::equivalent(pathSrc, pathDest)) {
+                        LogPrintf("cannot backup to wallet source file %s\n",
+                            pathDest.string());


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

jnewbery · 2017-09-21T15:43:10Z

test/functional/walletbackup.py

+
+        for sourcePath in sourcePaths:
+            assert_raises_jsonrpc(-4, "backup failed",
+                    self.nodes[0].backupwallet, sourcePath)


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

tomasvdw · 2017-09-21T17:17:59Z

@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 · 2017-09-21T19:16:23Z

Removed line breaks as per suggestion @jnewbery

sdaftuar · 2017-09-21T19:43:55Z

@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 · 2017-09-21T23:11:40Z

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 · 2017-09-22T00:10:21Z

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 · 2017-09-22T05:11:56Z

utACK b688b5242714f64c36e237b2c5c4151164ee6eb3

jonasschnelli · 2017-09-22T05:14:41Z

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 · 2017-09-22T11:35:02Z

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 · 2017-09-22T14:18:19Z

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 🙂

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 · 2017-09-25T23:23:20Z

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 · 2017-09-26T07:05:52Z

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 · 2017-09-26T17:25:56Z

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.

maflcko · 2017-10-09T19:08:47Z

test/functional/walletbackup.py

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


Needs rebase and replace with "assert_raises_rpc_error"

Done; rebased and changed to assert_rpc_error

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

TheBlueMatt · 2017-10-17T21:02:06Z

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

promag · 2017-10-17T21:25:19Z

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 · 2017-10-31T17:32:50Z

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

utACK

maflcko

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

maflcko · 2017-11-01T16:23:11Z

test/functional/walletbackup.py

@@ -190,6 +190,16 @@ def run_test(self):
        assert_equal(self.nodes[1].getbalance(), balance1)
        assert_equal(self.nodes[2].getbalance(), balance2)

+        # Backup to source wallet file must fail
+        sourcePaths = [


µnit: snake_case

…source file 5d465e3 Ensure backupwallet fails when attempting to backup to source file (Tomas van der Wansem) Pull request description: Previous behaviour was to destroy the wallet (to zero-length) This fixes #11375 Tree-SHA512: bfd1738659b15e3f23b6bbdf55ec12269c62c820bf701daec19500b52bd5845bb5516733c6f76f36197eb155182a8a35dc239ad4de2ef1e59bbb0f124a455759

Previous behaviour was to destroy the wallet (to zero-length) Github-Pull: bitcoin#11376 Rebased-From: 5d465e3

…kup to source file 5d465e3 Ensure backupwallet fails when attempting to backup to source file (Tomas van der Wansem) Pull request description: Previous behaviour was to destroy the wallet (to zero-length) This fixes bitcoin#11375 Tree-SHA512: bfd1738659b15e3f23b6bbdf55ec12269c62c820bf701daec19500b52bd5845bb5516733c6f76f36197eb155182a8a35dc239ad4de2ef1e59bbb0f124a455759

tomasvdw mentioned this pull request Sep 20, 2017

Backupwallet RPC call destroyed wallet if target equals the source wallet file #11375

Closed

fanquake added the Wallet label Sep 20, 2017

TheBlueMatt reviewed Sep 21, 2017

View reviewed changes

tomasvdw force-pushed the core branch from a673e6e to e062d58 Compare September 21, 2017 06:48

jnewbery reviewed Sep 21, 2017

View reviewed changes

tomasvdw force-pushed the core branch from e062d58 to b688b52 Compare September 21, 2017 19:15

jonasschnelli added the Needs backport label Sep 22, 2017

maflcko added this to the 0.15.1 milestone Oct 4, 2017

maflcko reviewed Oct 9, 2017

View reviewed changes

Ensure backupwallet fails when attempting to backup to source file

5d465e3

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

tomasvdw force-pushed the core branch from b688b52 to 5d465e3 Compare October 10, 2017 12:50

maflcko mentioned this pull request Nov 1, 2017

0.15: Backports #11592

Merged

maflcko reviewed Nov 1, 2017

View reviewed changes

maflcko merged commit 5d465e3 into bitcoin:master Nov 1, 2017

maflcko removed the Needs backport label Nov 1, 2017

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 1, 2017

Ensure backupwallet fails when attempting to backup to source file

fd79ed6

Previous behaviour was to destroy the wallet (to zero-length) Github-Pull: bitcoin#11376 Rebased-From: 5d465e3

dgenr8 mentioned this pull request Nov 13, 2017

Ensure backupwallet fails if pathDest is set to pathSource bitcoinxt/bitcoinxt#280

Merged

rnicoll mentioned this pull request Dec 26, 2017

Backupwallet RPC call destroyed wallet if target equals the source wallet file dogecoin/dogecoin#1402

Closed

bambache mentioned this pull request Jan 31, 2019

Merge #11376: Ensure backupwallet fails when attempting to backup to … projectpai/paicoin#219

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

tomasvdw commented Sep 20, 2017

Uh oh!

esotericnonsense commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esotericnonsense commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Sep 21, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

tomasvdw commented Sep 21, 2017

Uh oh!

sdaftuar commented Sep 21, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

tomasvdw Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

tomasvdw commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomasvdw commented Sep 21, 2017

Uh oh!

sdaftuar commented Sep 21, 2017

Uh oh!

gmaxwell commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Sep 22, 2017

Uh oh!

jonasschnelli commented Sep 22, 2017

Uh oh!

laanwj commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Sep 22, 2017

Uh oh!

gmaxwell commented Sep 25, 2017

Uh oh!

tomasvdw commented Sep 26, 2017

Uh oh!

jnewbery commented Sep 26, 2017

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasvdw Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 17, 2017

Uh oh!

promag commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar commented Oct 31, 2017

esotericnonsense commented Sep 21, 2017 •

edited

Loading

esotericnonsense commented Sep 21, 2017 •

edited

Loading

tomasvdw commented Sep 21, 2017 •

edited

Loading

gmaxwell commented Sep 21, 2017 •

edited

Loading

sdaftuar commented Sep 22, 2017 •

edited

Loading

laanwj commented Sep 22, 2017 •

edited

Loading

maflcko Oct 9, 2017 •

edited

Loading

promag commented Oct 17, 2017 •

edited

Loading