Skip to content

rpc: check for irregular files in dumptxoutset#17615

Closed
brakmic wants to merge 0 commit intobitcoin:masterfrom
brakmic:dumptxoutset-invalid-file-error
Closed

rpc: check for irregular files in dumptxoutset#17615
brakmic wants to merge 0 commit intobitcoin:masterfrom
brakmic:dumptxoutset-invalid-file-error

Conversation

@brakmic
Copy link
Contributor

@brakmic brakmic commented Nov 26, 2019 •
edited
Loading

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

@fanquake fanquake mentioned this pull request Nov 26, 2019
Closed
@ccdle12
Copy link
Contributor

ccdle12 commented Nov 26, 2019

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

@brakmic
Copy link
Contributor Author

brakmic commented Nov 26, 2019 •
edited
Loading

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.

@petterhs
Copy link

petterhs commented Nov 26, 2019

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

ubuntu 16

@brakmic
Copy link
Contributor Author

brakmic commented Nov 26, 2019

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

ubuntu 16

Thanks, will look into it.

@brakmic
Copy link
Contributor Author

brakmic commented Nov 26, 2019 •
edited
Loading

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.

fanquake
fanquake reviewed Nov 26, 2019
View reviewed changes
@brakmic
Copy link
Contributor Author

brakmic commented Nov 26, 2019 •
edited
Loading

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.

@petterhs
Copy link

petterhs commented Nov 26, 2019

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

@brakmic
Copy link
Contributor Author

brakmic commented Nov 26, 2019

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.

@laanwj
Copy link
Member

laanwj commented Nov 27, 2019 •
edited
Loading

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:
     * https://github.com/bitcoin/bitcoin/issues/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.

@brakmic
Copy link
Contributor Author

brakmic commented Nov 27, 2019

@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?

@laanwj
Copy link
Member

laanwj commented Nov 27, 2019

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.

@brakmic brakmic closed this Nov 27, 2019
@brakmic brakmic mentioned this pull request Nov 27, 2019
Closed
@brakmic
Copy link
Contributor Author

brakmic commented Nov 27, 2019

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.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@fanquake fanquake fanquake left review comments

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rpc: failure calling dumptxoutset with invalid filename

5 participants

@brakmic @ccdle12 @petterhs @laanwj @fanquake