Avoid unclean exit due to permissions issues when setting start on system startup

mruddy commented at 12:35 pm on April 27, 2022: contributor

moving https://github.com/bitcoin/bitcoin/pull/25000 to this repo

Closes https://github.com/bitcoin/bitcoin/issues/24953

This remedies two cases where the GUI can cause the process to exit in an unclean manner. These two cases are cases where the directory path causing the exception is likely/usually not under the node’s data and/or blocks directory and thus could be more likely to lack write permissions.

Instead of abruptly exiting the process, this patch instead makes log entries such as:

02022-04-27T11:43:03Z failed to create autostart directory: filesystem error: cannot create directories: Permission denied [/tmp/crash/autostart]

02022-04-27T11:44:05Z failed to remove autostart file: filesystem error: cannot remove: Permission denied [/tmp/crash/autostart/bitcoin.desktop]

the command line used to test and create the above log entries (with this patch applied) was:

0mkdir -p /tmp/btc && mkdir -p /tmp/crash && chmod 0500 /tmp/crash && XDG_CONFIG_HOME=/tmp/crash ./src/qt/bitcoin-qt -debug -datadir=/tmp/btc -connect=0

The short/easy process to generate the exception is noted in the original issue.

This PR does not address the other cases where fs::create_directories may create an unclean exit due to the reasoning that the other cases are generated by paths under the blocks and/or data directories and those directories need to be writable by the bitcoin process, or lots of things fail. This is a targeted PR to avoid a couple of unique recoverable situations.

hebasto renamed this:
~~gui: avoid unclean exit due to permissions issues when setting start on system startup~~
Avoid unclean exit due to permissions issues when setting start on system startup
on Apr 27, 2022

hebasto added the label Bug on Apr 27, 2022

luke-jr approved

luke-jr commented at 6:18 pm on May 8, 2022: member

utACK

shaavan approved

shaavan commented at 1:42 pm on May 14, 2022: contributor

ACK 933d2eddadd2b9ade2f3361f4bdc8fa7741d10f4

This PR introduces a try and catch statement to catch the filesystem_error. And adds a code block to handle this case correctly.
The code block first clearly describes the cause of the error, and the second returns a false bool value.

I was able to compile the PR and run GUI through it successfully.

RandyMcMillan commented at 9:00 pm on May 14, 2022: contributor

utACK 933d2ed

mruddy commented at 11:28 am on May 19, 2022: contributor

thanks to all of you for the code review. please let me know if there’s anything that i can do to move this pr forward.

jonatack commented at 8:49 am on May 20, 2022: contributor

Concept ACK and code looks reasonable, will review/test.

in src/qt/guiutil.cpp:620 in 933d2eddad outdated

617-        fs::remove(GetAutostartFilePath());
618+    if (!fAutoStart) {
619+        try {
620+            fs::remove(GetAutostartFilePath());
621+        } catch(const fs::filesystem_error& e) {
622+            LogPrintf("failed to remove autostart file: %s\n", e.what());

hebasto commented at 9:02 am on May 20, 2022:

Could you please Capitalize the first word in the log message to follow other LogPrintf() calls in the code base?

in src/qt/guiutil.cpp:635 in 933d2eddad outdated

628@@ -623,7 +629,12 @@ bool SetStartOnSystemStartup(bool fAutoStart)
629             return false;
630         pszExePath[r] = '\0';
631 
632-        fs::create_directories(GetAutostartDir());
633+        try {
634+            fs::create_directories(GetAutostartDir());
635+        } catch(const fs::filesystem_error& e) {
636+            LogPrintf("failed to create autostart directory: %s\n", e.what());

hebasto commented at 9:02 am on May 20, 2022:

The same.

hebasto changes_requested

gui: avoid unclean exit due to permissions issues when setting start on system startup

closes #24953

d932157eb7

mruddy commented at 12:23 pm on May 21, 2022: contributor

@hebasto Thanks for looking, I capitalized the first word in the log messages and force pushed it just now as d932157eb79a69555ba52ddc4acd37f16f9eaefc

hebasto added the label Linux on May 21, 2022

hebasto commented at 1:00 pm on May 21, 2022: member

Tested d932157eb79a69555ba52ddc4acd37f16f9eaefc on Ubuntu 22.04.

Instead of crashes I have nice log messages like that:

0Failed to remove autostart file: filesystem error: cannot remove: Permission denied [/tmp/crash/autostart/bitcoin-signet.desktop]

or

0Failed to create autostart directory: filesystem error: cannot create directories: Permission denied [/tmp/crash/autostart]

But users who run GUI could easily miss such log messages without a dedicated error message dialog.

mruddy commented at 1:43 pm on May 22, 2022: contributor

But users who run GUI could easily miss such log messages without a dedicated error message dialog.

Maybe, but I’m not too worried about that. I’m happy with the node not crashing and the log mention. If you have a message box that you want me to slap in there, I guess I could add it. If so, then do we need the log messages? It’s all the same to me as long as the node doesn’t crash and lose data.

hebasto commented at 2:06 pm on May 29, 2022: member

But users who run GUI could easily miss such log messages without a dedicated error message dialog.

Maybe, but I’m not too worried about that. I’m happy with the node not crashing and the log mention.

When users check the “Start Bitcoin Core on system login” checkbox, they naturally expect a certain behavior which could silently (with a log message only) fail due to permission issues. We cannot expect that all of the Bitcoin Core GUI users are tech-savvy enough to read a log file to find the reason of the failure.

I’d prefer a crash to behavior that will surely confuse users.

mruddy commented at 12:45 pm on May 31, 2022: contributor

fair enough. i’ll leave this open for a couple of weeks and if nobody wants it, i’ll close it and the associated issue.

mruddy closed this on Jun 9, 2022

mruddy deleted the branch on Jun 9, 2022

bitcoin-core locked this on Jun 9, 2023

Avoid unclean exit due to permissions issues when setting start on system startup #595