rpc: check for irregular files in dumptxoutset #17615

pull brakmic wants to merge 0 commits into bitcoin:master from brakmic:dumptxoutset-invalid-file-error changing 0 files +0 −0
  1. brakmic commented at 5:57 pm on November 26, 2019: contributor

    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

  2. fanquake added the label RPC/REST/ZMQ on Nov 26, 2019
  3. ccdle12 commented at 6:37 pm on November 26, 2019: contributor

    Hi @brakmic

    The functional tests fail for me on:

    osx

     02019-11-26T18:19:47.936000Z TestFramework (INFO): Initializing test directory /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag
 12019-11-26T18:19:49.122000Z TestFramework (ERROR): JSONRPC error
 2Traceback (most recent call last):
 3  File "/Users/Desktop/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
 4    self.run_test()
 5  File "rpc_dumptxoutset.py", line 27, in run_test
 6    out = node.dumptxoutset(FILENAME)
 7  File "/Users/Desktop/Projects/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
 8    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 9  File "/Users/Desktop/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
10    raise JSONRPCException(response['error'], status)
11test_framework.authproxy.JSONRPCException: /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag/node0/regtest/txoutset.dat is invalid (-8)
122019-11-26T18:19:49.176000Z TestFramework (INFO): Stopping nodes
132019-11-26T18:19:49.593000Z TestFramework (WARNING): Not cleaning up dir /var/folders/pq/0yxd9h7x0xx1ztz_n4qlnvk00000gn/T/bitcoin_func_test_u47a85ag
142019-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
152019-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

     02019-11-26T18:33:43.639000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_r77jxe_d
 12019-11-26T18:33:43.934000Z TestFramework (ERROR): JSONRPC error
 2Traceback (most recent call last):
 3  File "/home/Desktop/projects/personal/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
 4    self.run_test()
 5  File "rpc_dumptxoutset.py", line 27, in run_test
 6    out = node.dumptxoutset(FILENAME)
 7  File "/home/Desktop/projects/personal/bitcoin/test/functional/test_framework/coverage.py", line 47, in call
 8    return_val = self.auth_service_proxy_instance.call(args, *kwargs)
 9  File "/home/Desktop/projects/personal/bitcoin/test/functional/test_framework/authproxy.py", line 141, in call
10    raise JSONRPCException(response['error'], status)
11test_framework.authproxy.JSONRPCException: /tmp/bitcoin_func_test_r77jxe_d/node0/regtest/txoutset.dat is invalid (-8)
122019-11-26T18:33:43.985000Z TestFramework (INFO): Stopping nodes
132019-11-26T18:33:44.187000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_r77jxe_d
142019-11-26T18:33:44.187000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_r77jxe_d/test_framework.log
152019-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
  4. brakmic commented at 6:40 pm on November 26, 2019: contributor

    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.

  5. petterhs commented at 6:41 pm on November 26, 2019: none

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

    ubuntu 16

  6. brakmic commented at 6:43 pm on November 26, 2019: contributor

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

    ubuntu 16

    Thanks, will look into it.

  7. brakmic commented at 6:56 pm on November 26, 2019: contributor
    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.
  8. in src/rpc/blockchain.cpp:48 in 4adeb423f0 outdated 
    43@@ -44,6 +44,9 @@
44 #include <memory>
45 #include <mutex>
46 
47+#include <boost/filesystem/operations.hpp>
48+namespace boostfs = boost::filesystem;
    fanquake commented at 6:58 pm on November 26, 2019:
    If you are going to introduce new Boost Filesystem usage (try and avoid that if possible), it needs to be wrapped in our Boost Filesystem wrapper.
    brakmic commented at 6:58 pm on November 26, 2019:
    Ah, yes, of course. Will change it!
  9. brakmic commented at 7:17 pm on November 26, 2019: contributor

    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.

  10. petterhs commented at 7:36 pm on November 26, 2019: none
    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
  11. brakmic commented at 7:37 pm on November 26, 2019: contributor

    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.

  12. laanwj commented at 8:26 am on November 27, 2019: member

    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:

     0  fs::path filepath = request.params[0].get_str();
 1    filepath = fs::absolute(filepath);
 2
 3    /* Prevent arbitrary files from being overwritten. There have been reports
 4     * that users have overwritten wallet files this way:
 5     * [#9934](/bitcoin-bitcoin/9934/)
 6     * It may also avoid other security issues.
 7     */
 8    if (fs::exists(filepath)) {
 9        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");
10    }

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

  13. brakmic commented at 8:29 am on November 27, 2019: contributor

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

  14. laanwj commented at 8:53 am on November 27, 2019: member

    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.

  15. brakmic closed this on Nov 27, 2019
  16. brakmic commented at 10:05 pm on November 27, 2019: contributor
    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.
  17. MarcoFalke locked this on Dec 16, 2021


brakmic ccdle12 petterhs fanquake laanwj

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me