Add error handling to all boost filesystem functions #18189

pull uhliksk wants to merge 1 commits into bitcoin:master from bitcoin-boost-filesystem-error-handling:master changing 21 files +346 −117

uhliksk commented at 3:14 PM on February 21, 2020: none

All boost filesystem functions should be handled using error code to prevent random crashes caused by inaccesible files or directories. Increment in iterator should use separate error code variable because it have to be handled specifically to prevent infinite loop in diving to inaccessible directories.

To prevent coding errors in future the lint test is added to check if error code parameters are present.

Previous conversation about issue in #18095
Add error handling to all boost filesystem functions 615651ea82
DrahtBot added the label GUI on Feb 21, 2020
DrahtBot added the label RPC/REST/ZMQ on Feb 21, 2020
DrahtBot added the label Tests on Feb 21, 2020
DrahtBot added the label Utils/log/libs on Feb 21, 2020
DrahtBot added the label UTXO Db and Indexes on Feb 21, 2020
DrahtBot added the label Validation on Feb 21, 2020
DrahtBot added the label Wallet on Feb 21, 2020

in src/dbwrapper.cpp:136 in 615651ea82

 132 | @@ -133,7 +133,7 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo
 133 |              leveldb::Status result = leveldb::DestroyDB(path.string(), options);
 134 |              dbwrapper_private::HandleError(result);
 135 |          }
 136 | -        TryCreateDirectories(path);
 137 | +        assert(TryCreateDirectories(path));

practicalswift commented at 4:13 PM on February 21, 2020:

assert(…):s should not have side-effects :)

uhliksk commented at 5:05 PM on February 21, 2020:

Yes, sorry, I was debugging and I forgot to handle this correctly. I'll add proper error handling here in additional commit.

practicalswift commented at 4:14 PM on February 21, 2020: contributor

Concept ACK

What a great first-time contribution! Hope to see more contributions from you :)
DrahtBot commented at 5:28 PM on February 21, 2020: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #18526 (Remove PID file at the very end by hebasto)
- #17623 (rpc: add extensive file checks for dumptxoutset and dumpwallet by brakmic)
- #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
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.
DrahtBot cross-referenced this on Feb 21, 2020 from issue rpc: add extensive file checks for dumptxoutset and dumpwallet by brakmic
DrahtBot cross-referenced this on Feb 21, 2020 from issue Testschains: Many regtests with different genesis and default datadir by jtimon
DrahtBot cross-referenced this on Feb 21, 2020 from issue gui: Bilingual GUI error messages by hebasto
fanquake removed the label GUI on Feb 22, 2020
fanquake removed the label RPC/REST/ZMQ on Feb 22, 2020
fanquake removed the label Tests on Feb 22, 2020
fanquake removed the label UTXO Db and Indexes on Feb 22, 2020
fanquake removed the label Validation on Feb 22, 2020
fanquake removed the label Wallet on Feb 22, 2020
fanquake added the label Refactoring on Feb 22, 2020
promag commented at 12:32 AM on March 2, 2020: member

Concept ACK.
DrahtBot cross-referenced this on Apr 3, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101
DrahtBot cross-referenced this on Apr 5, 2020 from issue Remove PID file at the very end by hebasto

in src/init.cpp:289 in 615651ea82

 280 | @@ -281,12 +281,13 @@ void Shutdown(NodeContext& node)
 281 |      }
 282 |  #endif
 283 |  
 284 | -    try {
 285 | -        if (!fs::remove(GetPidFile())) {
 286 | -            LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);
 287 | -        }
 288 | -    } catch (const fs::filesystem_error& e) {
 289 | -        LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e));

MarcoFalke commented at 3:10 PM on April 10, 2020:

this seems fine as is (catching the error), no?

DrahtBot commented at 4:39 PM on April 10, 2020: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot added the label Needs rebase on Apr 10, 2020
promag cross-referenced this on May 13, 2020 from issue QT Crash when using open wallet dialog by instagibbs
adamjonas commented at 1:18 AM on May 20, 2020: member

Hi @uhliksk - pinging for rebase and response to question above.
uhliksk commented at 1:30 AM on May 22, 2020: none

Hi @adamjonas , sorry for delay, I'll be back on June 1st.
adamjonas commented at 5:07 PM on June 19, 2020: member

Hi @uhliksk, friendly monthly ping.
uhliksk commented at 9:47 PM on June 22, 2020: none

Hi @adamjonas, I'm alive but I had to postpone my work again. I'll be back soon.
fanquake commented at 5:09 AM on July 26, 2020: member

Going to close this for now. Let us know if/when you have time to work on it again and it can be re-opened.
fanquake closed this on Jul 26, 2020
uhliksk cross-referenced this on Sep 22, 2020 from issue Fix crashes and infinite loop in ListWalletDir() by uhliksk
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Refactoring Utils/log/libs Needs rebase

Linked (view graph)

#18095 Fix crashes and infinite loop in ListWalletDir()#18924 QT Crash when using open wallet dialog

Add error handling to all boost filesystem functions #18189

Conflicts