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

tylerchambers · 2021-07-30T18:29:15Z

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

Fixes #22571

src/util/settings.cpp

tryphe · 2021-07-31T03:46:39Z

Concept ACK

Zero-1729

tACK 2b07126

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:

shaavan

tACK 2b07126
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)

ON PR (after running src/qt/bitcoin-qt)

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 · 2021-08-04T18:00:32Z

Approach ACK

ghost

tACK 2b07126

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:

Error: Failed loading settings file:
- 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.

ryanofsky

Code review ACK 2b07126. Thanks for the fix! Note that PR bitcoin-core/gui#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)

ryanofsky · 2021-08-05T20:07:35Z

src/util/settings.cpp

    file.open(path);
-    if (!file.is_open()) return true; // Ok for file not to exist.
+    if (!file.is_open()) {
+      errors.emplace_back(strprintf("%s. Please check permissions.", path.string()));


In commit "error if settings.json exists, but is unreadable" (2b07126)

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:

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

Zero-1729 · 2021-08-05T22:38:42Z

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 bitcoin-core/gui#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:

$ src/qt/bitcoin-qt                                                   
Error: Settings file could not be read:
- /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

$ src/qt/bitcoin-qt
Error: Settings file could not be read:
- 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 · 2021-08-25T03:47:39Z

Quick summary to aid review: this PR is a non-gui version of bitcoin-core/gui#379 (merged)

theStack

ACK 2b07126 📁

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.

…adable 2b07126 error if settings.json exists, but is unreadable (Tyler Chambers) Pull request description: If settings.json exists, but is unreadable, we should error instead of overwriting. Fixes bitcoin#22571 ACKs for top commit: Zero-1729: tACK 2b07126 ShaMan239: tACK 2b07126 prayank23: tACK bitcoin@2b07126 ryanofsky: Code review ACK 2b07126. Thanks for the fix! Note that PR bitcoin-core/gui#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) theStack: ACK 2b07126 📁 Tree-SHA512: 6f7f96ce8a13213d0335198a2245d127264495c877105058d1503252435915b332a6e55068ac21088f4c0c017d564689f4956213328d5bdee81d73711efc5511

Github-Pull: bitcoin#22591 Rebased-From: 2b07126

Summary: This is a backport of [[bitcoin/bitcoin#22591 | core#22591]] Test Plan: `ninja all check-all` ``` $ sudo chmod a-rwx /bitcoinddata/settings.json $ src/bitcoind Error: Failed loading settings file: - /bitcoinddata/settings.json. Please check permissions. ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10783

…adable 2b07126 error if settings.json exists, but is unreadable (Tyler Chambers) Pull request description: If settings.json exists, but is unreadable, we should error instead of overwriting. Fixes bitcoin#22571 ACKs for top commit: Zero-1729: tACK 2b07126 ShaMan239: tACK 2b07126 prayank23: tACK bitcoin@2b07126 ryanofsky: Code review ACK 2b07126. Thanks for the fix! Note that PR bitcoin-core/gui#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) theStack: ACK 2b07126 📁 Tree-SHA512: 6f7f96ce8a13213d0335198a2245d127264495c877105058d1503252435915b332a6e55068ac21088f4c0c017d564689f4956213328d5bdee81d73711efc5511

ghost reviewed Jul 30, 2021

View reviewed changes

src/util/settings.cpp Outdated Show resolved Hide resolved

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

tryphe reviewed Jul 31, 2021

View reviewed changes

src/util/settings.cpp Outdated Show resolved Hide resolved

error if settings.json exists, but is unreadable

2b07126

fanquake requested a review from ryanofsky August 2, 2021 01:53

Zero-1729 approved these changes Aug 2, 2021

View reviewed changes

maflcko changed the title ~~Util: error if settings.json exists, but is unreadable.~~ Util: error if settings json exists, but is unreadable Aug 2, 2021

shaavan reviewed Aug 4, 2021

View reviewed changes

ghost approved these changes Aug 4, 2021

View reviewed changes

ryanofsky approved these changes Aug 5, 2021

View reviewed changes

ghost mentioned this pull request Aug 7, 2021

Prompt to reset settings when settings.json cannot be read bitcoin-core/gui#379

Merged

theStack approved these changes Sep 5, 2021

View reviewed changes

maflcko merged commit 91e07cc into bitcoin:master Sep 5, 2021

tylerchambers deleted the fix-22571 branch September 6, 2021 20:32

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

error if settings.json exists, but is unreadable

0beb4b7

Github-Pull: bitcoin#22591 Rebased-From: 2b07126

imacfan mentioned this pull request Nov 1, 2021

Disk usage issue 0.2.44 mynodebtc/mynode#589

Open

bitcoin locked as resolved and limited conversation to collaborators Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

tylerchambers commented Jul 30, 2021

Uh oh!

Uh oh!

Uh oh!

tryphe commented Jul 31, 2021

Uh oh!

Zero-1729 left a comment

Uh oh!

shaavan left a comment

Uh oh!

jonatack commented Aug 4, 2021

Uh oh!

ghost left a comment •

edited by ghost

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Aug 5, 2021

Uh oh!

Zero-1729 commented Aug 5, 2021 •

edited

Loading

Uh oh!

tryphe commented Aug 25, 2021 •

edited

Loading

Uh oh!

theStack left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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

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

Uh oh!

Conversation

tylerchambers commented Jul 30, 2021

Uh oh!

Uh oh!

Uh oh!

tryphe commented Jul 31, 2021

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 4, 2021

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

Zero-1729 commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Result

With @ryanofsky's suggestion above

Uh oh!

tryphe commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ghost left a comment •

edited by ghost

Loading

Zero-1729 commented Aug 5, 2021 •

edited

Loading

tryphe commented Aug 25, 2021 •

edited

Loading