rpc: check for irregular files in dumptxoutset #17615

This PR fixes the problem with handling of irregular file names described in #17612

It also defines a helper function CanWriteFile that is used by dumptxoutset and dumpwallet.

This function checks for:

• file name existence (if only a dir-path was given the execution stops with a corresponding JSON error)
• file name validity
• file name portability between POSIX and Windows
• write permissions
• file existence (if a file already exists the dump* operation stops, like it was done in previous versions)

The already existing JSON return objects and their messages have been preserved to prevent failing of tests and/or 3rd party tools whose parsers maybe rely on them.

Two tests have been added: in rpc_tests.cpp and test/functional/rpc_dumptxoutset.py

Closes: #17612

Hi @brakmic

The functional tests fail for me on:

osx

2019-11-26T18:19:47.936000Z TestFramework (INFO): Initializing test directory /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag
2019-11-26T18:19:49.122000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/Users/Desktop/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
    self.run_test()
  File "rpc_dumptxoutset.py", line 27, in run_test
    out = node.dumptxoutset(FILENAME)
  File "/Users/Desktop/Projects/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/Users/Desktop/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag/node0/regtest/txoutset.dat is invalid (-8)
2019-11-26T18:19:49.176000Z TestFramework (INFO): Stopping nodes
2019-11-26T18:19:49.593000Z TestFramework (WARNING): Not cleaning up dir /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag
2019-11-26T18:19:49.593000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag/test_framework.log
2019-11-26T18:19:49.594000Z TestFramework (ERROR): Hint: Call /Users/Desktop/Projects/bitcoin/test/functional/combine_logs.py '/var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag' to consolidate all logs

ubuntu 18

2019-11-26T18:33:43.639000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_r77jxe_d
2019-11-26T18:33:43.934000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/Desktop/projects/personal/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
    self.run_test()
  File "rpc_dumptxoutset.py", line 27, in run_test
    out = node.dumptxoutset(FILENAME)
  File "/home/Desktop/projects/personal/bitcoin/test/functional/test_framework/coverage.py", line 47, in call
    return_val = self.auth_service_proxy_instance.call(args, *kwargs)
  File "/home/Desktop/projects/personal/bitcoin/test/functional/test_framework/authproxy.py", line 141, in call
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: /tmp/bitcoin_func_test_r77jxe_d/node0/regtest/txoutset.dat is invalid (-8)
2019-11-26T18:33:43.985000Z TestFramework (INFO): Stopping nodes
2019-11-26T18:33:44.187000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_r77jxe_d
2019-11-26T18:33:44.187000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_r77jxe_d/test_framework.log
2019-11-26T18:33:44.188000Z TestFramework (ERROR): Hint: Call /home/Desktop/projects/personal/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_r77jxe_d' to consolidate all logs

Hi @brakmic

The functional tests fail for me on:

These are tests that are already included in rpc_dumptxoutset.py. The test I added executes when you comment out the other, failing, tests. As those haven't been coded by me, I didn't touch them. Of course, we should fix them, but in another PR, I think.

It throws for the example input: >bitcoin-cli dumptxoutset utxo.dat

ubuntu 16

It throws for the example input: >bitcoin-cli dumptxoutset utxo.dat

ubuntu 16

Thanks, will look into it.

As already suspected in my pre-PR message the standard is_regular_file wasn't enough and we had to switch to boost's portable_file_name. However, this function is actually much more powerful, because it helps a lot regarding portability.

Just to clarify the problem mentioned above by @petterhs

The previously used is_regular_file checks if a path is valid. Like: is this a file, or a pipe, a dir etc. This of course wasn't enough as we actually need to check for validity of a given string, before it becomes a path object.

Therefore, I switched to boost's portable_file_name.

--EDIT: Not sure, if this could become a problem, but portable_file_name forces you to use only those file names that are acceptable by various operating systems. In this case Linux, Windows, Mac. In a way, it disciplines you to use only "portable" names.

Is there an advantage to using if (!portable_file_name..) instead of checking after FILE* file{fsbridge::fopen(temppath, "wb")}; with if (!file){... like in #17614

Is there an advantage to using if (!portable_file_name..) instead of checking after FILE* file{fsbridge::fopen(temppath, "wb")}; with if (!file){... like in #17614

I'd say, it doesn't use raw pointers and C constructs like FILE.

Would make sense to have a RPC utility function (in rpc/util.cpp) for checking write file paths; it could check things such as:

• Is the path valid
• Does the file already exist (if so, reject)
• Is the path writable
• …

We have a similar check in dumpwallet:

  fs::path filepath = request.params[0].get_str();
    filepath = fs::absolute(filepath);

    /* Prevent arbitrary files from being overwritten. There have been reports
     * that users have overwritten wallet files this way:
     * [#9934](/github-metadata-backup-bitcoin-bitcoin/9934/)
     * It may also avoid other security issues.
     */
    if (fs::exists(filepath)) {
        throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.string() + " already exists. If you are sure this is what you want, move it out of the way first");
    }

As this function has the same risks, it'd make sense to share the same code.

@laanwj Indeed!

I'll then write such a utility function and substitute current checks with it.

Should I open a new PR, or is it OK to continue with it here?

I'll then write such a utility function and substitute current checks with it.

Thanks!

Should I open a new PR, or is it OK to continue with it here?

I'd prefer updating this one, it's close enough.

Moved to #17623 Reason: the PR got automatically closed after I tried to send squashed changes. There seems to be no way to reopen it again.