rpc: add extensive file checks for dumptxoutset and dumpwallet #17623

brakmic commented at 10:04 PM on November 27, 2019: contributor

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

It also defines a helper function EnsureFileWritable 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 for the current OS
write permissions
file existence (if a file already exists the dump* operation stops, like it was done in previous versions)

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

A functional test was added in test/functional/rpc_dumptxoutset.py

Closes: #17612

Notice: the original PR was #17615 but due to an error at squashing changes it got automatically closed by GitHub and could not be reopened again.

fanquake added the label RPC/REST/ZMQ on Nov 27, 2019

in src/rpc/util.cpp:754 in 0c61f62b1b outdated

 749 | @@ -749,3 +750,69 @@ UniValue GetServicesNames(ServiceFlags services)
 750 |  
 751 |      return servicesNames;
 752 |  }
 753 | +
 754 | +UniValue CanWriteFile(const std::string& filepath)

laanwj commented at 11:17 AM on November 28, 2019:

maybe rename to EnsureFileWritable(const std::string& filepath), then move the throws inside, returning void. This makes the call sites much cleaner, and there is precedent in other RPC utility functions such as EnsureMemPool EnsureWallet etc

brakmic commented at 11:23 AM on November 28, 2019:

Thanks. Will adapt the code.

in src/rpc/blockchain.cpp:2292 in 0c61f62b1b outdated

2284 | @@ -2285,17 +2285,17 @@ UniValue dumptxoutset(const JSONRPCRequest& request)
2285 |      }.Check(request);
2286 |  
2287 |      fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir());
2288 | +    UniValue check = CanWriteFile(path.string());
2289 | +    int code = find_value(check, "code").get_int();
2290 | +
2291 | +    if (code != 0) {
2292 | +      throw check;

practicalswift commented at 11:22 AM on November 28, 2019:

JSONRPCError is expected?

brakmic commented at 11:23 AM on November 28, 2019:

Yes, when nonzero.

in src/wallet/rpcdump.cpp:764 in 0c61f62b1b outdated

 761 | +    fs::path filepath = fs::absolute(request.params[0].get_str());
 762 | +    UniValue check = CanWriteFile(filepath.string());
 763 | +    int code = find_value(check, "code").get_int();
 764 | +
 765 | +    if (code != 0) {
 766 | +      throw check;

practicalswift commented at 11:22 AM on November 28, 2019:

Same here: JSONRPCError expected?

brakmic commented at 11:23 AM on November 28, 2019:

Same

brakmic commented at 11:25 AM on November 28, 2019:

But will now convert the function to return void and only throw, when irregular things happen (filename irregular, readonly mount etc.)

brakmic commented at 8:18 PM on December 4, 2019: contributor

As I'm not sure if I should open an issue on that, I'll put my findings about AbsPathForConfigVal from util/system.cpp here first.

My environment is macOS Catalina.

When I use the flag -debuglogfile=badfilename` it creates a debug file of that name, which would lead to problems on Windows, I assume.

With -debuglogfile=\badfile it creates a file named adfile. Here also, Windows might react differently.

With -debuglogfile=\bad\file it creates a file named ad.

Maybe I am exaggerating things here, because nobody would want to use such contrived names. However, there might be scenarios where someone is deliberately trying to disrupt the application (if possible via logging mechanism).

Otherwise, we should maybe think about reusing EnsureFileWritable here as well.

in src/rpc/util.cpp:754 in f6cfba6241 outdated

 749 | @@ -749,3 +750,64 @@ UniValue GetServicesNames(ServiceFlags services)
 750 |  
 751 |      return servicesNames;
 752 |  }
 753 | +
 754 | +void EnsureFileWritable(const std::string& filepath)

laanwj commented at 10:40 AM on December 5, 2019:

I think the code could be simplified by passing in a const fs::path& here directly, because all call-sites start from that and use .string().

brakmic commented at 10:42 AM on December 5, 2019:

Ok, will adapt it.

laanwj commented at 10:42 AM on December 5, 2019: member

Otherwise, we should maybe think about reusing EnsureFileWritable here as well.

Command line argument handling is a different issue—maybe one of the snazzy future command-line parsing frameworks could handle "filepath" arguments in a consistent way.

But please keep this PR in scope of the RPC interface.

in src/wallet/rpcdump.cpp:760 in 2b3e33175e outdated

 755 | @@ -756,8 +756,8 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 756 |  
 757 |      EnsureWalletIsUnlocked(pwallet);
 758 |  
 759 | -    fs::path filepath = request.params[0].get_str();
 760 | -    filepath = fs::absolute(filepath);
 761 | +    fs::path filepath = fs::absolute(request.params[0].get_str());
 762 | +    EnsureFileWritable(filepath.string());

laanwj commented at 11:31 AM on December 5, 2019:

can remove the .string() here now

brakmic commented at 11:33 AM on December 5, 2019:

Oops, I just removed a similar one from blockchain.cpp. Sorry. Will force push again. Thanks!

in src/test/rpc_tests.cpp:78 in 2b3e33175e outdated

  74 | @@ -75,6 +75,7 @@ BOOST_AUTO_TEST_CASE(rpc_rawparams)
  75 |      BOOST_CHECK_THROW(CallRPC("sendrawtransaction null"), std::runtime_error);
  76 |      BOOST_CHECK_THROW(CallRPC("sendrawtransaction DEADBEEF"), std::runtime_error);
  77 |      BOOST_CHECK_THROW(CallRPC(std::string("sendrawtransaction ")+rawtx+" extra"), std::runtime_error);
  78 | +    BOOST_CHECK_THROW(CallRPC(std::string("dumptxoutset utxo.dat`")), std::runtime_error);

laanwj commented at 11:35 AM on December 5, 2019:

Why does this generate a runtime error? I might be wrong but I think this test is for utility (no side-effects) RPCs only; no utxo set has been set up here, so I think this is unsafe

brakmic commented at 11:37 AM on December 5, 2019:

Well, I am unsure why it generates a runtime error, but I actually expected it to be generated because the file name is wrong. However, if a runtime error should not happen here, maybe I should look into it.

laanwj commented at 11:40 AM on December 5, 2019:

Isn't ` a valid filename character on pretty much all OSes?

That was not really my point though; I don't think this is the place to test this. The functional test is. I'd just remove this line.

brakmic commented at 11:43 AM on December 5, 2019:

Not sure if all OSes accept a backtick, but because I'm using boost's portable_name function it behaves more strictly towards non-portable file names. Here's the whole list of options that I've been using: https://www.boost.org/doc/libs/1_57_0/libs/filesystem/doc/portability_guide.htm We could go less or more strict, of course. I will then remove this test.

laanwj commented at 11:51 AM on December 5, 2019:

I think that's very much overkill. The filename only needs to be valid for the OS bitcoind is running on. Looking at that table, portable_posix_name (which is implied by portable_name) rules out using non-ASCII characters which is not good for internationalization!

brakmic commented at 11:54 AM on December 5, 2019:

Well, then I could use function native for that. My idea was to have a check that takes care of allowing only such names where one could create a file that could be copied to/from any OS. But, if this is too much I will make it less strict.

laanwj commented at 12:23 PM on December 5, 2019:

The problem is that boost's definition of "any OS" is way too broad. It would at most need to be compatible with modern operating systems that bitcoind runs on, not 80's UNIX or DOS. In any case, .native is better here.

brakmic closed this on Dec 5, 2019

brakmic reopened this on Dec 5, 2019

ryanofsky commented at 10:11 PM on December 13, 2019: member

I don't think this is a good approach. The EnsureFileWritable function is complex and has lots of holes in it. It isn't checking parent directory permissions properly (considering effective uid and gid and ACLs) so it will return true in some cases when a file isn't writable, and false in some cases when it is, and give a misleading error message. It isn't checking if the filesystem is mounted read only or just full and out of disk space.

I think a simpler approach that would fix #1712 and provide better feedback to users would be to check when fsbridge::fopen returns null and convert the GetErrorReason string into RPC error in that case.

brakmic commented at 11:49 PM on December 13, 2019: contributor

I don't think this is a good approach. The EnsureFileWritable function is complex and has lots of holes in it.

Yes, the current code is more complex than the variant I started with. And as filesystems are anything but portable the code could become even more complex if I tried to "emulate" heterogenous filesystems' properties (windows ACLs, Linux permissions, readonly mounts etc.).

In the end, the important part is to move the file checks out of the two call sites, dumptxoutset and dumpwallet. Maybe the solution could be a combination of your idea plus some additional bits regarding irregular file names,...under the umbrella of EnsureFileWritable?

I could try to find a way to include parent-directory permission-checks and readonly mounts, but I think it's better to work on less complex solutions.

If there are more effective, less complex ideas, I'll gladly change the code. Or just close this PR.

Regards,

ryanofsky commented at 12:19 AM on December 14, 2019: member

I just don't understand in what ways the current PR is better than a simple one-line fix:

if (!file) throw JSONRPCError(RPC_MISC_ERROR, strprintf("Cannot create %s (%s)", path.string(), GetErrorReason()));

The current PR seems more more complicated, less reliable, and worse for users if it is going generate misleading error messages instead of passing on the actual error from the operating system.

In the end, the important part is to move the file checks out of the two call sites, dumptxoutset and dumpwallet.

I don't understand why this is important or even a useful thing to do.

Maybe the solution could be a combination of your idea plus some additional bits regarding irregular file names

I'm also unclear what is the use-case for restricting characters in file names instead of just passing the provided path to the operating system.

ryanofsky commented at 12:32 AM on December 14, 2019: member

I didn't realize GetErrorReason isn't currently implemented on windows:

https://github.com/bitcoin/bitcoin/blob/5728f88d645c124b980ecb6b6943a94a1ad46612/src/fs.cpp#L27-L29

So my suggestion would be more than a one line fix, and would require tweaking fsbridge::fopen to return an error string or code (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=vs-2019).

Still I think this would be a simpler approach, and that it would be better to return the actual filesystem error to users rather than try to anticipate that an error might happen before it actually does, generate our own errors, and do work the operating system should be doing for us

brakmic commented at 7:23 PM on December 14, 2019: contributor

I have expanded the checks for possible errors (readonly-fs, non-existing paths, permission-denied, no-space-left-on-device, and general IO errors).

laanwj commented at 12:12 PM on December 15, 2019: member

Still I think this would be a simpler approach, and that it would be better to return the actual filesystem error to users rather than try to anticipate that an error might happen before it actually does, generate our own errors, and do work the operating system should be doing for us

I agree with this in concept. To be clear, why I went along initially, is that I didn't see EnsureFileWritable as an exhaustive list of things that can go wrong, but just a way to tell the user about (common) problems in a more friendly way.

It also rules out overwriting an existing file, which is risky and not covered by any OS checks.

But it's growing and growing and it looks like @brakmic is trying to rule out any file-system issue that can potentially happen. It's not possible to second-guess the operating system like that (and if it's possible, it's not maintainable). The only way to be sure, in the end, is to try.

I'm also unclear what is the use-case for restricting characters in file names instead of just passing the provided path to the operating system.

True.

DrahtBot commented at 11:09 PM on January 1, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18689 (rpc: allow dumptxoutset to dump human-readable data by pierreN)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

rpc: added helper function used for file checks in dumptxoutset and dumpwallet eb3100f392

brakmic closed this on May 10, 2020

DrahtBot locked this on Feb 15, 2022