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.

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.

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.

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,

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.

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

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

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

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.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• rpc: allow dumptxoutset to dump human-readable data #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.