Util: error if settings json exists, but is unreadable

tylerchambers commented at 6:29 pm on July 30, 2021: contributor

If settings.json exists, but is unreadable, we should error instead of overwriting.

in src/util/settings.cpp:70 in 4b267cfe29 outdated

66+
67     fsbridge::ifstream file;
68     file.open(path);
69-    if (!file.is_open()) return true; // Ok for file not to exist.
70+    if (!file.is_open()) {
71+      errors.emplace_back(strprintf("Unable to read settings file %s. Are the permissions correct?", path.string()));

unknown commented at 6:41 pm on July 30, 2021:

Concept ACK.

nit: Maybe error message can be changed to

0-Are the permissions correct?
1+Please check permissions.

tylerchambers commented at 1:37 pm on July 31, 2021:

I think this is a good call. The final error now presents like this:

DrahtBot added the label Utils/log/libs on Jul 30, 2021

in src/util/settings.cpp:65 in 4b267cfe29 outdated

59@@ -60,9 +60,16 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
60     values.clear();
61     errors.clear();
62 
63+    if (!fs::exists(path)) {
64+        return true; // Ok for file to not exist
65+    }

tryphe commented at 3:44 am on July 31, 2021:

This should be a single line as per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c (bullet point 7)

Also, the comment should be above the line(even though there are lines that stray away from this):

0// Ok for file to not exist
1if (!fs::exists(path)) return true;

Also consider adding a comment to the change on line 69 to make it more clear (although the error message is kind of self explanatory)

MarcoFalke commented at 6:04 am on July 31, 2021:

Bullet point 7 says it can be on a single line, but it doesn’t have to

tylerchambers commented at 1:37 pm on July 31, 2021:

I think that aids readability. Fixed.

tryphe commented at 3:46 am on July 31, 2021: contributor

Concept ACK

error if settings.json exists, but is unreadable 2b071265c3

fanquake requested review from ryanofsky on Aug 2, 2021

Zero-1729 approved

Zero-1729 commented at 5:02 am on August 2, 2021: contributor

tACK 2b071265c37da22f15769945fd159b50a14792a3

Tested on macOS v11.4

Can confirm patch works as advertised in the op, running bitcoin-qt reports a file permission error if settings.json can’t be read instead of overwriting the file (as is currently done on master).

On master

Running bitcoin-qt after changing the file ownership & permissions of settings.json to root & -rwx------ (via chmod 700 settings.json) respectively, opens without an error and overwrites settings.json.

After patch

However, running bitcoin-qt after changing file ownership to root and file permissions to -rwx------, the permissions error is reported, and the file is no longer overwritten:

MarcoFalke renamed this:
~~Util: error if settings.json exists, but is unreadable.~~
Util: error if settings json exists, but is unreadable
on Aug 2, 2021

shaavan commented at 5:49 pm on August 4, 2021: contributor

tACK 2b071265c37da22f15769945fd159b50a14792a3 Compiled and Tested on Ubuntu 20.04

This PR adds the functionality to display an error message when the settings.json file exists but could not be read instead of simply going forward and overwriting it. This is done by returning False result (instead of True, as done by the master) when the file is there but could not be opened. Tested the PR by navigating to the .bitcoin folder followed by sudo chmod root:root settings.json and sudo chmod a-rwx settings.json as suggested here. The PR is successful in doing what is said in the description. Here is the screenshot of the successful working of the PR.

ON MASTER (after running src/qt/bitcoin-qt) Screenshot from 2021-08-04 20-46-18

ON PR (after running src/qt/bitcoin-qt) Screenshot from 2021-08-04 20-27-30

I agree with the changes suggested by op. Because it might happen that the user might have added some custom changes to the settings.json file, the user could lose those changes if, for some reason, the file was not able to be read at the time of starting of bitcoin-qt. Also, the user should be aware of when the Client opened the settings file correctly and when it was overwritten because the settings file was not read. This PR can tackle both of these issues.

jonatack commented at 6:00 pm on August 4, 2021: member

Approach ACK

unknown approved

unknown commented at 7:29 pm on August 4, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/22591/commits/2b071265c37da22f15769945fd159b50a14792a3

Tried on Windows because normally such things behave differently in Windows resulting in some issues. Changed the permissions for settings.json file and tried opening bitcoin-qt.

Master branch:

This created a file: settings.json.tmp:

PR branch:

nit: Couldn’t find anything in debug.log. Maybe we can print this error in logs as well. Or do we require -debug=something for it?

Same error is printed if I try to run bitcoind:

0Error: Failed loading settings file:
1- E:\regtest\settings.json. Please check permissions.

Also tried running bitcoind and bitcoin-qt with file open in other programs (editors, defender etc.) but didn’t see any errors or unexpected behavior.

I am not sure if there can be more reasons for is_open() returning false. ‘Please check permissions’ looks good enough for now as a hint in the error or most likely reason for the error.

in src/util/settings.cpp:69 in 2b071265c3

65+
66     fsbridge::ifstream file;
67     file.open(path);
68-    if (!file.is_open()) return true; // Ok for file not to exist.
69+    if (!file.is_open()) {
70+      errors.emplace_back(strprintf("%s. Please check permissions.", path.string()));

ryanofsky commented at 8:07 pm on August 5, 2021:

In commit “error if settings.json exists, but is unreadable” (2b071265c37da22f15769945fd159b50a14792a3)

Indent should be 4 instead of 2 spaces

Also this text is pretty inconsistent with the other errors below. The other errors mention what problem happened at the path, while this just prints the path with no description of the failure, only a request to check permissions. I didn’t really follow the previous review discussion, but for clarity and consistency with other errors below I’d consider changing to something like:

0errors.emplace_back(strprintf("Failed to open settings file %s for reading (may be inaccessible due to permissions)", path.string()));

ryanofsky approved

ryanofsky commented at 8:24 pm on August 5, 2021: member

Code review ACK 2b071265c37da22f15769945fd159b50a14792a3. Thanks for the fix! Note that PR https://github.com/bitcoin-core/gui/pull/379 will change the appearance of dialogs shown in screenshots above. So it could be interesting to test the two PRs together (but current testing seems more than sufficient)

Zero-1729 commented at 10:38 pm on August 5, 2021: contributor

Following the comment above, I was curious to see how the two PRs would look once both are merged. So I decided to rebase this PR atop https://github.com/bitcoin-core/gui/pull/379 and test.

I also tested with @ryanofsky’s suggestion above (result added below). After seeing the errors side-by-side, @tylerchambers I’ll also recommend considering including the suggestion.

Result

Tested on macOS v11.4

As I did in my previous test, I ran bitcoin-qt after changing file ownership to root and file permissions to -rwx------, then got the following cli error and GUI prompt dialog:

0$ src/qt/bitcoin-qt                                                   
1Error: Settings file could not be read:
2- /Users/abubakar/Library/Application Support/Bitcoin/settings.json. Please check permissions.

Note: Now the settings.json is only overwritten if the user explicitly clicks reset, else it remains untouched. IMHO, the new GUI prompt is a lot sleeker and less noisy.

After opening the details:

With @ryanofsky’s suggestion above

0$ src/qt/bitcoin-qt
1Error: Settings file could not be read:
2- Failed to open settings file /Users/abubakar/Library/Application Support/Bitcoin/settings.json for reading (may be inaccessible due to permissions)

I am really glad to see how these two PRs work well to tackle this issue! 🍾

tryphe commented at 3:47 am on August 25, 2021: contributor

Quick summary to aid review: this PR is a non-gui version of https://github.com/bitcoin-core/gui/pull/379 (merged)

theStack approved

theStack commented at 12:30 pm on September 5, 2021: member

ACK 2b071265c37da22f15769945fd159b50a14792a3 📁

Tested by first removing permissions via chmod 000 ~/.bitcoin/settings.json and then starting bitcoind. In master, the file is overwritten, in the PR branch, the introduced error message appears and bitcoind exits with error code 1.

MarcoFalke merged this on Sep 5, 2021

MarcoFalke closed this on Sep 5, 2021

tylerchambers deleted the branch on Sep 6, 2021

sidhujag referenced this in commit 64186eccd8 on Sep 7, 2021

luke-jr referenced this in commit 0beb4b73d9 on Oct 10, 2021

Fabcien referenced this in commit ddb02862f8 on Jan 10, 2022

PastaPastaPasta referenced this in commit 8d07123fa7 on Jul 17, 2022

PastaPastaPasta referenced this in commit befba8f2c3 on Aug 30, 2022

PastaPastaPasta referenced this in commit 657bfaebcd on Aug 30, 2022

PastaPastaPasta referenced this in commit ab4405258e on Sep 3, 2022

DrahtBot locked this on Sep 6, 2022

Util: error if settings json exists, but is unreadable #22591

Result

With @ryanofsky’s suggestion above