util: explicitly close all AutoFiles that have been written #29307

pull vasild wants to merge 4 commits into bitcoin:master from vasild:AutoFile_error_check changing 20 files +195 −54
  1. vasild commented at 2:35 pm on January 24, 2024: contributor

    fclose(3) may fail to flush the previously written data to disk, thus a failing fclose(3) is as serious as a failing fwrite(3).

    Previously the code ignored fclose(3) failures. This PR improves that by changing all users of AutoFile that use it to write data to explicitly close the file and handle a possible error.

    Other alternatives are:

    1. fflush(3) after each write to the file (and throw if it fails from the AutoFile::write() method) and hope that fclose(3) will then always succeed. Assert that it succeeds from the destructor :roll_eyes:. Will hurt performance.
    2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
    3. (this is implemented in the latest incarnation of this PR) Redesign AutoFile so that its destructor cannot fail. Adjust all its users :sob:. For example, if the file has been written to, then require the callers to explicitly call the AutoFile::fclose() method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for FILE* which automatically closes the file when it goes out of scope and there are a lot of users of AutoFile.
    4. Pass a new callback function to the AutoFile constructor which will be called from the destructor to handle fclose() errors, as described in #29307 (comment). My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling AutoFile::fclose() instead of getting the AutoFile object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed fclose()).
  2. DrahtBot commented at 2:35 pm on January 24, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29307.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, l0rinc, achow101
    Concept ACK ryanofsky
    Stale ACK maflcko

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32748 (fees: prevent redundant estimates flushes by ismaelsadeeq)
    • #31560 (rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script by theStack)
    • #31144 ([IBD] multi-byte block obfuscation by l0rinc)
    • #28792 (Embed default ASMap as binary dump header file by fjahr)

    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.

  3. DrahtBot added the label Utils/log/libs on Jan 24, 2024
  4. vasild force-pushed on Jan 24, 2024
  5. DrahtBot added the label CI failed on Jan 24, 2024
  6. DrahtBot commented at 2:58 pm on January 24, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/20819752683

  7. in src/streams.cpp:18 in 7c58f6ea4f outdated 
    14     if (!m_file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr");
15     if (m_xor.empty()) {
16-        return std::fread(dst.data(), 1, dst.size(), m_file);
17+        const auto n = std::fread(dst.data(), 1, dst.size(), m_file);
18+        if (ferror(m_file)) {
19+            throw std::ios_base::failure(strprintf("AutoFile::detail_fread: %s", SysErrorString(errno)));
    maflcko commented at 3:59 pm on January 24, 2024:

    not sure. detail_fread is an implementation detail and thin wrapper, with the goal to simply return the return value of std::fread. It should be left for the caller to decide about the error.

    In any case, if you want to do this, you’d have to apply it to both branches of the if. The error message shouldn’t differ just because a file is xor’d or not.

    Finally, it would be good to explain whether this is a bugfix or a refactor/belt-and-suspenders. I don’t think an error/eof could have occurred if the full dst.size() was read. So I don’t think this is needed, but I could be wrong. Ideally, there is a unit test showing why the change is needed.

    vasild commented at 8:40 am on January 25, 2024:

    Alright, moved to the caller.

    IMO this is a bugfix, but I am not 100% sure either. Might as well be viewed as “belt-and-suspenders”. What is certain is that it can’t hurt even if there is never an error on any platform if detail_fread(dst) == dst.size(). A unit test would require simulating an IO error under the fread(3) call and identically behaving fread(3) on all platforms.

  8. in src/test/fuzz/autofile.cpp:50 in 7c58f6ea4f outdated 
    46@@ -47,7 +47,7 @@ FUZZ_TARGET(autofile)
47                 }
48             },
49             [&] {
50-                auto_file.fclose();
51+                assert(auto_file.fclose() == 0);
    vasild commented at 5:18 pm on January 24, 2024:

    This assert actually failed in CI (wow!):

    https://cirrus-ci.com/task/5586893237125120

     0Run autofile with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/autofile')]INFO: Running with entropic power schedule (0xFF, 100).
 1INFO: Seed: 3765617163
 2INFO: Loaded 1 modules   (547059 inline 8-bit counters): 547059 [0x55cd54dd29b8, 0x55cd54e582ab), 
 3INFO: Loaded 1 PC tables (547059 PCs): 547059 [0x55cd54e582b0,0x55cd556b11e0), 
 4INFO:      535 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/autofile
 5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1041296 bytes
 6INFO: seed corpus: files: 535 min: 1b max: 1041296b total: 17489928b rss: 106Mb
 7fuzz: test/fuzz/autofile.cpp:50: auto autofile_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `auto_file.fclose() == 0' failed.
 8==18378== ERROR: libFuzzer: deadly signal
 9    [#0](/bitcoin-bitcoin/0/) 0x55cd5193ab55 in __sanitizer_print_stack_trace (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a5fb55) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
10    [#1](/bitcoin-bitcoin/1/) 0x55cd518922fc in fuzzer::PrintStackTrace() (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19b72fc) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
11    [#2](/bitcoin-bitcoin/2/) 0x55cd51878367 in fuzzer::Fuzzer::CrashCallback() (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199d367) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
12    [#3](/bitcoin-bitcoin/3/) 0x7fb162f1d8ff  (/lib/x86_64-linux-gnu/libc.so.6+0x428ff) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
13    [#4](/bitcoin-bitcoin/4/) 0x7fb162f7498a in pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x9998a) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
14    [#5](/bitcoin-bitcoin/5/) 0x7fb162f1d855 in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42855) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
15    [#6](/bitcoin-bitcoin/6/) 0x7fb162f018b6 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x268b6) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
16    [#7](/bitcoin-bitcoin/7/) 0x7fb162f017da  (/lib/x86_64-linux-gnu/libc.so.6+0x267da) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
17    [#8](/bitcoin-bitcoin/8/) 0x7fb162f14185 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x39185) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
18    [#9](/bitcoin-bitcoin/9/) 0x55cd51accacc in autofile_fuzz_target(Span<unsigned char const>)::$_3::operator()() const src/test/fuzz/autofile.cpp:50:17
19    [#10](/bitcoin-bitcoin/10/) 0x55cd51accacc in unsigned long CallOneOf<autofile_fuzz_target(Span<unsigned char const>)::$_0, autofile_fuzz_target(Span<unsigned char const>)::$_1, autofile_fuzz_target(Span<unsigned char const>)::$_2, autofile_fuzz_target(Span<unsigned char const>)::$_3, autofile_fuzz_target(Span<unsigned char const>)::$_4, autofile_fuzz_target(Span<unsigned char const>)::$_5>(FuzzedDataProvider&, autofile_fuzz_target(Span<unsigned char const>)::$_0, autofile_fuzz_target(Span<unsigned char const>)::$_1, autofile_fuzz_target(Span<unsigned char const>)::$_2, autofile_fuzz_target(Span<unsigned char const>)::$_3, autofile_fuzz_target(Span<unsigned char const>)::$_4, autofile_fuzz_target(Span<unsigned char const>)::$_5) src/./test/fuzz/util.h:42:27
20    [#11](/bitcoin-bitcoin/11/) 0x55cd51accacc in autofile_fuzz_target(Span<unsigned char const>) src/test/fuzz/autofile.cpp:27:9
21    [#12](/bitcoin-bitcoin/12/) 0x55cd5204a52c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
22    [#13](/bitcoin-bitcoin/13/) 0x55cd5204a52c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
23    [#14](/bitcoin-bitcoin/14/) 0x55cd51879814 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199e814) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
24    [#15](/bitcoin-bitcoin/15/) 0x55cd51878f09 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199df09) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
25    [#16](/bitcoin-bitcoin/16/) 0x55cd5187aaf6 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199faf6) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
26    [#17](/bitcoin-bitcoin/17/) 0x55cd5187b017 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19a0017) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
27    [#18](/bitcoin-bitcoin/18/) 0x55cd5186875b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x198d75b) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
28    [#19](/bitcoin-bitcoin/19/) 0x55cd51892ce6 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19b7ce6) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
29    [#20](/bitcoin-bitcoin/20/) 0x7fb162f030cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
30    [#21](/bitcoin-bitcoin/21/) 0x7fb162f03188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
31    [#22](/bitcoin-bitcoin/22/) 0x55cd5185d624 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1982624) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
32NOTE: libFuzzer has rudimentary signal handlers.
33      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
34SUMMARY: libFuzzer: deadly signal
35MS: 0 ; base unit: 0000000000000000000000000000000000000000
36artifact_prefix='./'; Test unit written to ./crash-276f4f9d3e6ba98f897bd1082aec7214ca8e4f5d
37INFO: Running with entropic power schedule (0xFF, 100).
38INFO: Seed: 3765617163
39INFO: Loaded 1 modules   (547059 inline 8-bit counters): 547059 [0x55cd54dd29b8, 0x55cd54e582ab), 
40INFO: Loaded 1 PC tables (547059 PCs): 547059 [0x55cd54e582b0,0x55cd556b11e0), 
41INFO:      535 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/autofile
42INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1041296 bytes
43INFO: seed corpus: files: 535 min: 1b max: 1041296b total: 17489928b rss: 106Mb
44fuzz: test/fuzz/autofile.cpp:50: auto autofile_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `auto_file.fclose() == 0' failed.
45==18378== ERROR: libFuzzer: deadly signal
46    [#0](/bitcoin-bitcoin/0/) 0x55cd5193ab55 in __sanitizer_print_stack_trace (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1a5fb55) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
47    [#1](/bitcoin-bitcoin/1/) 0x55cd518922fc in fuzzer::PrintStackTrace() (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19b72fc) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
48    [#2](/bitcoin-bitcoin/2/) 0x55cd51878367 in fuzzer::Fuzzer::CrashCallback() (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199d367) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
49    [#3](/bitcoin-bitcoin/3/) 0x7fb162f1d8ff  (/lib/x86_64-linux-gnu/libc.so.6+0x428ff) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
50    [#4](/bitcoin-bitcoin/4/) 0x7fb162f7498a in pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x9998a) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
51    [#5](/bitcoin-bitcoin/5/) 0x7fb162f1d855 in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x42855) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
52    [#6](/bitcoin-bitcoin/6/) 0x7fb162f018b6 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x268b6) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
53    [#7](/bitcoin-bitcoin/7/) 0x7fb162f017da  (/lib/x86_64-linux-gnu/libc.so.6+0x267da) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
54    [#8](/bitcoin-bitcoin/8/) 0x7fb162f14185 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x39185) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
55    [#9](/bitcoin-bitcoin/9/) 0x55cd51accacc in autofile_fuzz_target(Span<unsigned char const>)::$_3::operator()() const src/test/fuzz/autofile.cpp:50:17
56    [#10](/bitcoin-bitcoin/10/) 0x55cd51accacc in unsigned long CallOneOf<autofile_fuzz_target(Span<unsigned char const>)::$_0, autofile_fuzz_target(Span<unsigned char const>)::$_1, autofile_fuzz_target(Span<unsigned char const>)::$_2, autofile_fuzz_target(Span<unsigned char const>)::$_3, autofile_fuzz_target(Span<unsigned char const>)::$_4, autofile_fuzz_target(Span<unsigned char const>)::$_5>(FuzzedDataProvider&, autofile_fuzz_target(Span<unsigned char const>)::$_0, autofile_fuzz_target(Span<unsigned char const>)::$_1, autofile_fuzz_target(Span<unsigned char const>)::$_2, autofile_fuzz_target(Span<unsigned char const>)::$_3, autofile_fuzz_target(Span<unsigned char const>)::$_4, autofile_fuzz_target(Span<unsigned char const>)::$_5) src/./test/fuzz/util.h:42:27
57    [#11](/bitcoin-bitcoin/11/) 0x55cd51accacc in autofile_fuzz_target(Span<unsigned char const>) src/test/fuzz/autofile.cpp:27:9
58    [#12](/bitcoin-bitcoin/12/) 0x55cd5204a52c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
59    [#13](/bitcoin-bitcoin/13/) 0x55cd5204a52c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
60    [#14](/bitcoin-bitcoin/14/) 0x55cd51879814 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199e814) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
61    [#15](/bitcoin-bitcoin/15/) 0x55cd51878f09 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199df09) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
62    [#16](/bitcoin-bitcoin/16/) 0x55cd5187aaf6 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199faf6) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
63    [#17](/bitcoin-bitcoin/17/) 0x55cd5187b017 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19a0017) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
64    [#18](/bitcoin-bitcoin/18/) 0x55cd5186875b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x198d75b) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
65    [#19](/bitcoin-bitcoin/19/) 0x55cd51892ce6 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19b7ce6) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
66    [#20](/bitcoin-bitcoin/20/) 0x7fb162f030cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
67    [#21](/bitcoin-bitcoin/21/) 0x7fb162f03188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
68    [#22](/bitcoin-bitcoin/22/) 0x55cd5185d624 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1982624) (BuildId: 34c8e9fbac2706ae466ade7d359adebec5d82bcf)
69NOTE: libFuzzer has rudimentary signal handlers.
70      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
71SUMMARY: libFuzzer: deadly signal
72MS: 0 ; base unit: 0000000000000000000000000000000000000000
73artifact_prefix='./'; Test unit written to ./crash-276f4f9d3e6ba98f897bd1082aec7214ca8e4f5d
74Target ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/autofile')] failed with exit code 77

    No base64 of the unit in the output, I guess I cannot access ./crash-276f4f9d3e6ba98f897bd1082aec7214ca8e4f5d and I cannot reproduce by running FUZZ=autofile ./src/test/fuzz/fuzz -runs=1000000 .../qa-assets/fuzz_seed_corpus/autofile locally. Maybe the CI machine ran out of disk space?

    vasild commented at 8:22 am on January 25, 2024:

    I realized that auto_file used in this fuzz test is not associated with a regular file on the filesystem, but with a “cookie” returned by fuzzed_file_provider.open() returned by fopencookie(3) which uses a custom close function FuzzedFileProvider::close() which can arbitrarily return -1:

    https://github.com/bitcoin/bitcoin/blob/207220ce8b767d8efdb5abf042ecf23d846ded73/src/test/fuzz/util.cpp#L352

    changed this to ignore the return value of auto_file.fclose().

  9. vasild force-pushed on Jan 25, 2024
  10. DrahtBot removed the label CI failed on Jan 25, 2024
  11. maflcko commented at 11:32 am on January 29, 2024: member

    I don’t think any care close-check needs to be done when reading from a file.

    So what about removing the write method from AutoFile, and introduce a new derived class to add it back. This class could Assume that the file was flushed/closed before the destructor is called?

  12. vasild commented at 10:48 am on February 4, 2024: contributor

    I don’t think any care close-check needs to be done when reading from a file.

    That is my understanding too.

    So what about removing the write method from AutoFile, and introduce a new derived class to add it back. This class could Assume that the file was flushed/closed before the destructor is called?

    Assume is more for code correctness, not for external errors (like IO error). If it does not fail during testing and on CI, that does not mean IO errors are absent and will not occur on the users’ machines. Given that Assume() is removed from production builds, it still means that file corruption could remain unnoticed on live environments, like now on master.

  13. maflcko commented at 9:45 am on February 5, 2024: member

    This class could Assume that the file was flushed/closed before the destructor is called?

    Assume is more for code correctness, not for external errors (like IO error).

    The assumption would be that the close was called before the destructor is called. This is a code correctness question and seems like the perfect fit for Assume. That is, if the Assume fails, the code was missing a close, and the code must be changed to fix the internal bug. The Assume does not in any way check for IO errors.

  14. vasild commented at 3:17 pm on February 5, 2024: contributor

    Then I misunderstood this:

    This class could Assume that the file was flushed/closed before the destructor is called

    How?

  15. maflcko commented at 3:34 pm on February 5, 2024: member
    AutoFile holds a file, which will be nullptr when it is closed. So if the file is nullptr, then it can be assumed to be flushed.
  16. vasild commented at 4:40 pm on February 5, 2024: contributor
    How/when would the file be closed?
  17. maflcko commented at 8:00 am on February 6, 2024: member

    How/when would the file be closed?

    An AutoFile can be closed by calling the fclose() method in the code.

  18. vasild commented at 9:45 am on February 6, 2024: contributor
    Ok, the users of the class then have to explicitly close. That is 1.4. from the OP, I elaborated it with more words.
  19. DrahtBot commented at 3:42 pm on March 12, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/20850328376

  20. DrahtBot added the label CI failed on Mar 12, 2024
  21. Retropex referenced this in commit f54c172ffe on Mar 28, 2024
  22. Retropex referenced this in commit 70403c5bb7 on Mar 28, 2024
  23. achow101 commented at 3:43 pm on April 9, 2024: member
    Seems like there is some discussion in the description that is a better fit for a brainstorming issue.
  24. DrahtBot marked this as a draft on Apr 23, 2024
  25. DrahtBot commented at 6:50 am on April 23, 2024: contributor
    Converted to draft due to failing CI and inactivity
  26. vasild force-pushed on May 10, 2024
  27. vasild commented at 10:28 am on May 10, 2024: contributor

    5543990321...1fa61d9edc: rebase and log a message from ~AutoFile() if closing fails and terminate the program.

    Ready for review. I think this would be a strict improvement over what we have now in master - a silent ignoring of a possible file corruption.

  28. vasild marked this as ready for review on May 10, 2024
  29. vasild force-pushed on May 10, 2024
  30. Sjors commented at 4:59 pm on May 10, 2024: member
    @vasild CI seems unhappy.
  31. vasild force-pushed on May 13, 2024
  32. vasild marked this as a draft on May 13, 2024
  33. vasild force-pushed on May 13, 2024
  34. DrahtBot removed the label CI failed on May 13, 2024
  35. vasild marked this as ready for review on May 14, 2024
  36. vasild commented at 9:51 am on May 14, 2024: contributor
    Fixed the CI!
  37. in src/streams.cpp:28 in de23848eed outdated 
    24@@ -23,8 +25,9 @@ std::size_t AutoFile::detail_fread(Span<std::byte> dst)
25 
26 void AutoFile::read(Span<std::byte> dst)
27 {
28-    if (detail_fread(dst) != dst.size()) {
29-        throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");
30+    if (detail_fread(dst) != dst.size() || ferror(m_file)) {
    maflcko commented at 10:57 am on May 14, 2024:

    de23848eed5437f5e4dc6ccdc38b40cc58738ead: I still don’t understand this. Why would fread succeed to read the requested number of bytes, but then mark the stream with an error? If the goal is to somehow detect errors after the read succeeded, I am not sure this should be a goal. Otherwise, it would be good to explain what real user-facing bug this fixes or introduces.

    Either this is a refactor, in which case it seems pointless, or it is a behavior change, in which case it needs to explain why it beneficial and correct.

    vasild commented at 12:39 pm on May 14, 2024:

    This was somewhat answered in #29307 (review) above but let me elaborate - the thing that prompted me to do this change is this text from the fread(3) manual page:

    The function fread() does not distinguish between end-of-file and error, and callers must use feof(3) and ferror(3) to determine which occurred.

    I think checking for ferror(3) after fread(3) is a kind of harmless “belt-and-suspenders”. If you are sure that if detail_fread(dst) == dst.size() then ferror(m_file) will always be false or feel strongly against this then I could drop the commit de23848eed util: check for error after reading from a file.

    maflcko commented at 5:32 pm on May 23, 2024:

    If you are sure that if detail_fread(dst) == dst.size() then ferror(m_file) will always be false or feel strongly against this then I could drop the commit https://github.com/bitcoin/bitcoin/commit/de23848eed5437f5e4dc6ccdc38b40cc58738ead util: check for error after reading from a file.

    I don’t feel strongly in one way or another. I am just saying that with the currently available information, it is not possible to confidently review this and know it is a good change. At a minimum, it should be clear whether this is a refactor or a behavior change, and what user-facing bug this could possibly prevent or fix. (I can’t think of one, but maybe I am missing something?)

    vasild commented at 7:00 am on June 20, 2024:

    Well, it is pretty much obvious that checking for an error after read is either:

    • a noop, if the read could have never failed because it managed to read as many bytes as requested or
    • a bug fix where a read error was ignored before the fix

    so this is a safe change (it does not break anything), possibly fixing a bug. Given that you keep pushing against this I will drop this commit in the hopes to move this PR forward.

    maflcko commented at 7:08 am on June 20, 2024:
    No, I don’t understand why a “read error was ignored before the fix” and why “this is a safe change (it does not break anything)”. For example, if this was hit on a filesystem that stored a block file, which was fully and successfully read, but then return an error (for an unknown reason), this would mean that for a user where the program previously worked fine without any issues, is now unusable (for an unknown reason).
    vasild commented at 8:14 am on June 20, 2024:

    I just found a similar code to this proposed change:

    https://github.com/bitcoin/bitcoin/blob/2d21060af831fa8f8f1791b4464961d5f28a88cb/src/node/utxo_snapshot.cpp#L74-L80

    After reading from a file and getting everything that is needed from it, the code checks if there is an extra data and if that read fails it reports an error.

    maflcko commented at 8:30 am on June 20, 2024:
    The code you linked to is primarily checking for unexpected trailing data and only printing a warning. It is not making the program possibly unusable.
    vasild commented at 10:05 am on June 20, 2024:

    I think I see your point, you prefer 1. over 2., assuming we got all the bytes we asked for from a read, then:

    1. Ignore possible IO error by not checking for it
    2. Check for an error and behave as if the read failed

    I dropped this change.

  38. vasild force-pushed on May 14, 2024
  39. in src/kernel/mempool_persist.cpp:221 in f9cf6b516b outdated 
    217@@ -213,6 +218,7 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
218                   fs::file_size(dump_path));
219     } catch (const std::exception& e) {
220         LogInfo("Failed to dump mempool: %s. Continuing anyway.\n", e.what());
221+        (void)file.fclose();
    maflcko commented at 12:54 pm on May 15, 2024:

    Not sure I understand this change either. This seems to be mixing the third alternative in the pull request description with the approach you wanted to take?

    Defeats the purpose of a RAII wrapper for FILE* which automatically closes the file when it goes out of scope and there are a lot of users of AutoFile.

    vasild commented at 1:11 pm on May 23, 2024:

    It is best if all users of AutoFile explicitly call the fclose() method and check that it succeeds (option 3. from PR description). This is not feasible because there are many users of AutoFile. However, some of those users already do call the fclose() method explicitly. For those users it is easy and I added checks whether fclose() succeeds. DumpMempool() is such a user and the change is:

    0             throw std::runtime_error("FileCommit failed");
1-        file.fclose();           
2+        if (file.fclose() != 0) {    
3+            throw std::runtime_error(
4+                strprintf("Error closing %s: %s", fs::PathToString(file_fspath), SysErrorString(errno)));
5+        }
6         if (!RenameOver(dump_path + ".new", dump_path)) {

    For consistency in DumpMempool() I changed all code paths in that function to explicitly call fclose(). There are two codepaths and this added (void)file.fclose(); is the other one. I ignore the return value because this is already failure handling and the function is going to return false. I didn’t want to do something like “while handling error1, error2 occurred”.

    maflcko commented at 5:25 pm on May 23, 2024:

    Oh, now I see this is required, because otherwise the node would crash on this error instead of continuing with the current code, if this was missing?

    Not sure this is desirable, see also https://github.com/bitcoin/bitcoin/pull/29307/files#r1612065514

    vasild commented at 10:09 am on June 20, 2024:
    Resolving this discussion. Changed the semantic of AutoFile to require all write users to call fclose().
  40. DrahtBot added the label Needs rebase on May 23, 2024
  41. in src/streams.h:413 in de23848eed outdated 
    410-    ~AutoFile() { fclose(); }
411+    ~AutoFile()
412+    {
413+        if (fclose() != 0 && m_was_written) {
414+            LogPrintLevel(BCLog::ALL, BCLog::Level::Error, "Error closing file: %s\n", SysErrorString(errno));
415+            std::abort();
    maflcko commented at 5:22 pm on May 23, 2024:

    not sure about unconditionally crashing the node on any failure here. This may or may not be an issue, so the call site will have to decide about this.

    For example, If an RPC user dumps the mempool to an external (corrupt) USB thumb drive, a failure shouldn’t crash the node, but notify the user over the RPC.

    Also, I am not sure about involving logging in this low-level code. Logging may not be set up, so this may never be printed before the crash.

    Also, I am not sure about the log message. It just says that there was an error, but not whose fault it was or what should be done about it.

    A better solution overall may be to just call if (m_was_written) Assume(IsNull());. This should highlight all violating call sites during CI, because the test should be deterministic. (If there is a code path that is not deterministically tested, all hopes are lost anyway, and I don’t think the solution should be to crash the users node completely and without considering the context?)

    vasild commented at 10:07 am on June 20, 2024:

    if (m_was_written) Assume(IsNull());

    Done.

  42. luke-jr referenced this in commit ec8ef5e51c on Jun 13, 2024
  43. luke-jr referenced this in commit 17f1f24993 on Jun 13, 2024
  44. vasild force-pushed on Jun 20, 2024
  45. vasild renamed this:
    util: check for errors after close and read in AutoFile
    util: explicitly close all AutoFiles that have been written
    on Jun 20, 2024
  46. DrahtBot removed the label Needs rebase on Jun 20, 2024
  47. maflcko commented at 12:04 pm on June 20, 2024: member

    lgtm ACK 11be9f4103ea219f801a1f0fe1385f66ca70ad22

    Will do some testing later

  48. luke-jr referenced this in commit d2ea6ab60b on Jun 22, 2024
  49. DrahtBot added the label Needs rebase on Jul 2, 2024
  50. vasild force-pushed on Jul 2, 2024
  51. vasild commented at 12:41 pm on July 2, 2024: contributor
    11be9f4103...ea89a6e368: rebase due to conflicts
  52. DrahtBot removed the label Needs rebase on Jul 2, 2024
  53. DrahtBot added the label Needs rebase on Jul 9, 2024
  54. vasild force-pushed on Jul 22, 2024
  55. vasild commented at 10:33 am on July 22, 2024: contributor
    ea89a6e368...4533445fd7: rebase due to conflicts
  56. DrahtBot added the label CI failed on Jul 22, 2024
  57. DrahtBot commented at 12:13 pm on July 22, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27742621741

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  58. DrahtBot removed the label Needs rebase on Jul 22, 2024
  59. vasild commented at 12:56 pm on July 22, 2024: contributor
    CI failure seems unrelated.
  60. DrahtBot removed the label CI failed on Jul 22, 2024
  61. ryanofsky commented at 9:26 pm on July 22, 2024: contributor

    Concept ACK, approach ~0

    The PR description is a bit vague about how it is changing the AutoFile API.

    Previously, the autofile API allowed but did not require AutoFile callers to check for failing fclose calls after writing. Now it requires that callers call the fclose method manually and explicitly use or discard its return value.

    Checking for failing fclose calls is important because if a call fails, it probably indicates that there is a serious problem that should trigger a log message or an alert. Specifically if the file was used for writing it could mean that data could not successfully be saved to disk. So it would be bad for fclose errors to be discarded without being handled at all or reported somewhere.

    The PR makes AutoFile require callers to handle failing fclose calls, and it enforces this in two ways:

    1. It adds [[nodiscard]] annotation to the fclose method so its return value cannot be ignored. The return value is 0 if successful or EOF (-1) if unsuccessful.
    2. It tries to enforce that callers actually invoke fclose by adding an Assume statement to the destructor that terminates the program with a fatal error if the file was written to and fclose was not called and the binary was built in a debug mode.

    It seems like this approach works, but it is maybe a little fragile because it doesn’t actually enforce that caller handles the fclose result in every code path. So for example if an early return statement is added after a caller opens an autofile object but before it calls the fclose method, and the early return code path is not normally executed, it can introduce a latent bug that occasionally will crash debug builds, while providing no indication that anything is wrong in release builds.

    I think if the goal is to require AutoFile callers to handle failing fclose calls, a more reliable way to do that would be to change the constructor signature to

    0explicit AutoFile(std::FILE* file, std::function<void()> on_error);

    And require the caller to provide an on_error callback that does error handing like

    0[&] {
1        remove(pathTmp);
2        LogError("%s: Failed to close file %s: %s\n", __func__, fs::PathToString(pathTmp), SysErrorString(errno));
3}

    I think the current approach is ok too, but it seems more fragile because it doesn’t detect all programming errors, and when it does it detects them with a runtime crash instead of a compile time error.

    I am coming to this late, so if you are really attached to the current approach I can just review and ack it, but I wanted to suggest an alternative.

  62. vasild commented at 1:13 pm on July 23, 2024: contributor

    @ryanofsky, I am not so attached to the current approach :) if it can be improved, then lets improve it.

    For the on_error callback - that would be called on fclose() error from destructor only, right? Not on any error? E.g. the write() method throws an exception on error.

    Not all callers have the file name when creating AutoFile which means we would have to log some generic message without a file name. Or go an extra mile and make sure we have the file name, but some callers don’t use files at all, IIRC the fuzzer uses some cookie “files” that are not on the filesystem.

    Then the callback can be omitted and the message can be logged from the destructor:

     0     ~AutoFile()
 1     {   
 2         if (m_was_written) {
 3             // Callers that wrote to the file must have closed it explicitly
 4             // with the fclose() method and checked that the close succeeded.
 5             // This is because here from the destructor we have no way to signal
 6             // error due to close which, after write, could mean the file is
 7             // corrupted and must be handled properly at the call site.
 8             Assume(IsNull());
 9         }
10                        
11-        (void)fclose();
12+        if (fclose() != 0) {
13+            LogError("Failed to close file: %s\n", SysErrorString(errno));
14+        }
15     }
  63. DrahtBot added the label CI failed on Aug 7, 2024
  64. DrahtBot added the label Needs rebase on Aug 21, 2024
  65. vasild force-pushed on Aug 22, 2024
  66. vasild commented at 9:27 am on August 22, 2024: contributor
    4533445fd7...1dcd626fe1: rebase due to conflicts
  67. DrahtBot removed the label Needs rebase on Aug 22, 2024
  68. vasild force-pushed on Aug 23, 2024
  69. vasild commented at 9:25 am on August 23, 2024: contributor
    1dcd626fe1...587d996f07: rebase, get the extra printout from #29307 (comment) and resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/28052
  70. DrahtBot removed the label CI failed on Aug 23, 2024
  71. vasild force-pushed on Sep 2, 2024
  72. vasild commented at 2:54 pm on September 2, 2024: contributor
    587d996f07...489c2477e7: rebase to pick CMake
  73. DrahtBot added the label CI failed on Sep 5, 2024
  74. in src/addrdb.cpp:84 in 489c2477e7 outdated 
    83         return false;
84     }
85-    fileout.fclose();
86+    if (fileout.fclose() != 0) {
87+        remove(pathTmp);
88+        LogError("%s: Failed to close file %s: %s\n", __func__, fs::PathToString(pathTmp), SysErrorString(errno));
    hodlinator commented at 9:07 pm on September 7, 2024:

    Less error-prone to use the error value returned from fclose() rather than relying on global errno which might have been mutated by remove() etc.

    0    if (int err{fileout.fclose()}; err != 0) {
1        remove(pathTmp);
2        LogError("Failed to close file %s: %s", fs::PathToString(pathTmp), SysErrorString(err));

    (Also dropped pre--logsourcelocations __func__ and removed superfluous newline).

    vasild commented at 12:27 pm on April 14, 2025:
    Done. Note however that fclose() returns either 0 or EOF (-1). It does not return the error code.
  75. in src/index/blockfilterindex.cpp:234 in 489c2477e7 outdated 
    228@@ -229,6 +229,12 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
229     }
230 
231     fileout << filter.GetBlockHash() << filter.GetEncodedFilter();
232+
233+    if (fileout.fclose() != 0) {
234+        LogPrintf("%s: Failed to close filter file %d\n", __func__, pos.nFile);
    hodlinator commented at 9:31 pm on September 7, 2024:

    Could use SystemErr(err) here?

    0    if (int err{fileout.fclose()}; err != 0) {
1        LogPrintf("Failed to close filter file %d: %s", pos.nFile, SysErrorString(err));

    Could be done in other files too (net.cpp, blockstorage.cpp). (I can understand not wanting to do it further above in this specific file though).

    vasild commented at 12:28 pm on April 14, 2025:
    Done, modulo fclose() does not return the error code which is to be found in errno.
  76. in src/bench/streams_findbyte.cpp:31 in 489c2477e7 outdated 
    27@@ -28,7 +28,7 @@ static void FindByte(benchmark::Bench& bench)
28     });
29 
30     // Cleanup
31-    file.fclose();
32+    (void)file.fclose();
    hodlinator commented at 9:35 pm on September 7, 2024:

    Rather than silence errors, it might be good to at least assert against errors here and elsewhere in benchmarks/tests as we want to ensure we are benchmarking/testing properly? (Not for some fuzz-cases though as you already found out).

    0    assert(file.fclose() != 0);

    That’s already how you handled src/wallet/test/fuzz/wallet_bdb_parser.cpp.

    vasild commented at 12:28 pm on April 14, 2025:
    Done, but it should be ==, not !=.
    vasild commented at 9:40 am on May 22, 2025:

    Note that this here is in a bench test where we operate on a real file and an error to close it means something really went wrong badly, e.g. a disk IO error. Such a condition should stop the test with a failure.

    In contrast, in fuzz tests some of the “files” we operate on are fuzzed files which mean they are not really backed on disk and their operations are mocked to sometimes return an error, e.g. to simulate a broken disk. Those mocked errors are to be expected and should not make the test fail.

    Those fuzz mocks are done via FuzzedFileProvider::close() which is set by FuzzedFileProvider::open() as a close callback of the mocked FILE* that is returned.

  77. DrahtBot removed the label CI failed on Sep 15, 2024
  78. DrahtBot added the label Needs rebase on Sep 17, 2024
  79. vasild force-pushed on Sep 28, 2024
  80. vasild commented at 5:20 am on September 28, 2024: contributor
    489c2477e7...dba7835386: rebase due to conflicts
  81. DrahtBot removed the label Needs rebase on Sep 28, 2024
  82. in src/policy/fees.cpp:966 in dba7835386 outdated 
    949@@ -950,7 +950,7 @@ void CBlockPolicyEstimator::Flush() {
950 void CBlockPolicyEstimator::FlushFeeEstimates()
951 {
952     AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")};
953-    if (est_file.IsNull() || !Write(est_file)) {
954+    if (est_file.IsNull() || !Write(est_file) || est_file.fclose() != 0) {
955         LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
    hodlinator commented at 7:28 pm on January 21, 2025:

    nit: Might take the opportunity to improve grammar and modernize log:

    0        LogWarning("Failed to write fee estimates to %s. Continuing anyway.", fs::PathToString(m_estimation_filepath));
    vasild commented at 12:28 pm on April 14, 2025:
    Done.
  83. in src/rpc/blockchain.cpp:53 in dba7835386 outdated 
    46@@ -47,6 +47,7 @@
47 #include <util/check.h>
48 #include <util/fs.h>
49 #include <util/strencodings.h>
50+#include <util/syserror.h>
51 #include <util/translation.h>
52 #include <validation.h>
53 #include <validationinterface.h>
    hodlinator commented at 8:27 pm on January 21, 2025:

    Should also explicitly fclose() afile from line 2790/2791.

    https://github.com/bitcoin/bitcoin/blob/dba783538683cfc6af209c640c2d019648493f31/src/rpc/blockchain.cpp#L2790-L2791

    vasild commented at 12:24 pm on April 14, 2025:
    It is being closed inside WriteUTXOSnapshot().
    hodlinator commented at 8:57 am on May 20, 2025:

    nit: Would be nice if WriteUTXOSnapshot() took AutoFile&& afile to signify a transfer of ownership.

     0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 1index 0fa66eb211..0acd8c3e98 100644
 2--- a/src/rpc/blockchain.cpp
 3+++ b/src/rpc/blockchain.cpp
 4diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 5index 0fa66eb211..0acd8c3e98 100644
 6--- a/src/rpc/blockchain.cpp
 7+++ b/src/rpc/blockchain.cpp
 8@@ -83,7 +83,7 @@ UniValue WriteUTXOSnapshot(
 9     CCoinsViewCursor* pcursor,
10     CCoinsStats* maybe_stats,
11     const CBlockIndex* tip,
12-    AutoFile& afile,
13+    AutoFile&& afile,
14     const fs::path& path,
15     const fs::path& temppath,
16     const std::function<void()>& interruption_point = {});
17@@ -3091,7 +3091,7 @@ static RPCHelpMan dumptxoutset()
18         }
19     }
20 
21-    UniValue result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point);
22+    UniValue result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, std::move(afile), path, temppath, node.rpc_interruption_point);
23     fs::rename(temppath, path);
24 
25     result.pushKV("path", path.utf8string());
26@@ -3143,7 +3143,7 @@ UniValue WriteUTXOSnapshot(
27     CCoinsViewCursor* pcursor,
28     CCoinsStats* maybe_stats,
29     const CBlockIndex* tip,
30-    AutoFile& afile,
31+    AutoFile&& afile,
32     const fs::path& path,
33     const fs::path& temppath,
34     const std::function<void()>& interruption_point)
35@@ -3220,12 +3220,12 @@ UniValue WriteUTXOSnapshot(
36 UniValue CreateUTXOSnapshot(
37     node::NodeContext& node,
38     Chainstate& chainstate,
39-    AutoFile& afile,
40+    AutoFile&& afile,
41     const fs::path& path,
42     const fs::path& tmppath)
43 {
44     auto [cursor, stats, tip]{WITH_LOCK(::cs_main, return PrepareUTXOSnapshot(chainstate, node.rpc_interruption_point))};
45-    return WriteUTXOSnapshot(chainstate, cursor.get(), &stats, tip, afile, path, tmppath, node.rpc_interruption_point);
46+    return WriteUTXOSnapshot(chainstate, cursor.get(), &stats, tip, std::move(afile), path, tmppath, node.rpc_interruption_point);
47 }
48 
49 static RPCHelpMan loadtxoutset()
50diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h
51index 954ede6519..423fd49ff0 100644
52--- a/src/rpc/blockchain.h
53+++ b/src/rpc/blockchain.h
54@@ -51,7 +51,7 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
55 UniValue CreateUTXOSnapshot(
56     node::NodeContext& node,
57     Chainstate& chainstate,
58-    AutoFile& afile,
59+    AutoFile&& afile,
60     const fs::path& path,
61     const fs::path& tmppath);
62 
63diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
64index eebf504d98..841ea4ba63 100644
65--- a/src/test/util/chainstate.h
66+++ b/src/test/util/chainstate.h
67@@ -48,7 +48,7 @@ CreateAndActivateUTXOSnapshot(
68     AutoFile auto_outfile{outfile};
69 
70     UniValue result = CreateUTXOSnapshot(
71-        node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); // Will close auto_outfile.
72+        node, node.chainman->ActiveChainstate(), std::move(auto_outfile), snapshot_path, snapshot_path); // Will close auto_outfile.
73     LogPrintf(
74         "Wrote UTXO snapshot to %s: %s\n", fs::PathToString(snapshot_path.make_preferred()), result.write());
    vasild commented at 11:00 am on May 22, 2025:
    Done, patch taken. Thanks!
  84. hodlinator commented at 10:56 am on January 22, 2025: contributor

    Concept ACK

    Appreciate the intent of being rigorous in reporting issues with flushing file content to disk.

    Alternative implementation

    The current PR which produces a run-time failure in the AutoFile destructor upon forgetting to explicitly call fclose(). I attempted an alternative implementation (branch) inspired by ryanofsky’s comment that takes a lambda in the constructor which is called upon fclose()-failure in the destructor. This enforces the context-aware handling of the issue at compile-time. The change was somewhat invasive but I’m curious to hear what others make of it. (It also splits AutoFile into FileReader and FileWriter (naming inspired by other types in streams.h)).

  85. DrahtBot added the label Needs rebase on Jan 22, 2025
  86. luke-jr referenced this in commit 06105d8da8 on Feb 26, 2025
  87. vasild force-pushed on Apr 14, 2025
  88. DrahtBot removed the label Needs rebase on Apr 14, 2025
  89. vasild commented at 12:25 pm on April 14, 2025: contributor
    dba7835...9c16900: rebase due to conflicts
  90. vasild force-pushed on Apr 14, 2025
  91. vasild commented at 12:25 pm on April 14, 2025: contributor
    9c16900b9b...8939947449: address suggestions
  92. vasild commented at 12:34 pm on April 14, 2025: contributor

    lambda in the constructor which is called upon fclose()-failure in the destructor … I’m curious to hear what others make of it. (It also splits AutoFile into FileReader and FileWriter (naming inspired by other types in streams.h)).

    Concept ACK, but I am worried that the lack of review interest here would mean that an even more invasive patch would never make it.

  93. l0rinc commented at 1:30 pm on April 14, 2025: contributor
    Concept ACK
  94. DrahtBot added the label Needs rebase on Apr 16, 2025
  95. vasild force-pushed on Apr 17, 2025
  96. vasild commented at 4:40 am on April 17, 2025: contributor
    8939947449...aa88c6dfb6: rebase due to conflicts
  97. DrahtBot added the label CI failed on Apr 17, 2025
  98. DrahtBot commented at 5:45 am on April 17, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40701339509

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  99. DrahtBot removed the label Needs rebase on Apr 17, 2025
  100. vasild force-pushed on Apr 17, 2025
  101. vasild commented at 7:32 am on April 17, 2025: contributor
    aa88c6dfb6...0e97a68e13: fix CI
  102. in src/node/blockstorage.cpp:1122 in 0e97a68e13 outdated 
    1118+        LogPrintLevel(BCLog::BLOCKSTORAGE,
1119+                      BCLog::Level::Error,
1120+                      "Failed to close block file %s: %s",
1121+                      pos.ToString(),
1122+                      SysErrorString(errno));
1123+        m_opts.notifications.fatalError(_("Failed close file when writing block."));
    DrahtBot commented at 7:46 am on April 17, 2025:

    (✨ LLM generated, experimental)

    “Failed close file when writing block.” → “Failed to close file when writing block.”
  103. vasild force-pushed on Apr 17, 2025
  104. vasild commented at 8:17 am on April 17, 2025: contributor
    0e97a68e13...1f71111e21: fix AI and CI (even more)
  105. in src/net.cpp:4018 in 1f71111e21 outdated 
    4011@@ -4012,6 +4012,11 @@ static void CaptureMessageToFile(const CAddress& addr,
4012     uint32_t size = data.size();
4013     ser_writedata32(f, size);
4014     f << data;
4015+
4016+    if (f.fclose() != 0) {
4017+        throw std::ios_base::failure(
4018+            strprintf("Error closing %s after write, file contents is likely incomplete", fs::PathToString(path)));
    DrahtBot commented at 8:22 am on April 17, 2025:

    (✨ LLM generated, experimental)

    file contents is likely incomplete ⇒ file contents are likely incomplete

  106. DrahtBot removed the label CI failed on Apr 17, 2025
  107. in src/index/blockfilterindex.cpp:154 in 1f71111e21 outdated 
    150@@ -151,7 +151,7 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
151         LogError("%s: Failed to open filter file %d\n", __func__, pos.nFile);
152         return false;
153     }
154-    if (!file.Commit()) {
155+    if (!file.Commit() || file.fclose() != 0) {
    l0rinc commented at 11:09 am on April 17, 2025:

    If we can’t fully commit to disk, we won’t call fclose directly - we’ll rely on the destructor’s close. We’d get a commitment failure message (though that could have been successful) - and maybe another message from the destructor.

    Could we separate the two errors?

    0if (!file.Commit()) {
1    LogError("%s: Failed to commit filter file %d", __func__, pos.nFile);
2    return false;
3}
4if (file.fclose()) {
5    LogError("%s: Failed to close filter file %d", __func__, pos.nFile);
6    return false;
7}

    or unify them to a single error like we’re doing in https://github.com/bitcoin/bitcoin/pull/29307/files?w=1#diff-7194b705e3c48e34fbd6e648fbbee39c6e0b49ee9658658b930678a9a2d31dd6R957

    0if (file.IsNull() || !file.Commit() || file.fclose()) {
1    LogError("%s: Failed to flush filter file %d", __func__, pos.nFile);
2    return false;
3}
    vasild commented at 2:29 pm on May 2, 2025:
    There is even a bigger problem with if (!file.Commit() || file.fclose() != 0) { – if the commit fails when fclose() will not be called, it will return and the file destructor will be upset that the file has been written to and is not closed. Fixed.
  108. in src/index/blockfilterindex.cpp:253 in 1f71111e21 outdated 
    228@@ -229,6 +229,12 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
229     }
230 
231     fileout << filter.GetBlockHash() << filter.GetEncodedFilter();
232+
233+    if (fileout.fclose() != 0) {
234+        LogPrintf("Failed to close filter file %d: %s", pos.nFile, SysErrorString(errno));
235+        return 0;
236+    }
    l0rinc commented at 11:23 am on April 17, 2025:

    Why is this a LogInfo, shouldn’t it be a LogError?

    0    if (fileout.fclose()) {
1        LogError("%s: Failed to close filter file %d: %s", __func__, pos.nFile, SysErrorString(errno));
2        return 0;
3    }
    vasild commented at 2:30 pm on May 2, 2025:
    Changed to LogError(), but __func__ is redundant with the config option -logsourcelocations, so I omitted it.
  109. in src/node/blockstorage.cpp:965 in 1f71111e21 outdated 
    971+                    fileout << blockundo << hasher.GetHash();
972+                }
973             }
974 
975-            fileout.flush(); // Make sure `AutoFile`/`BufferedWriter` go out of scope before we call `FlushUndoFile`
976+            if (file.fclose() != 0) {
    l0rinc commented at 11:28 am on April 17, 2025:
    nit: now that we’re closing explicitly we probably don’t need the scope, too (wouldn’t we double-log in that case anyway?) Or if we keep it, could make sense to add the closing inside - unless it’s deliberate, in which case we might want to explain it in the commit message
    vasild commented at 2:31 pm on May 2, 2025:
    Right. Removed the scope.
  110. in src/node/blockstorage.cpp:973 in 1f71111e21 outdated 
    979+                              "Failed to close block undo file %s: %s",
980+                              pos.ToString(),
981+                              SysErrorString(errno));
982+                return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
983+            }
984+            // Make sure that the file is closed before we call `FlushUndoFile`.
    l0rinc commented at 11:28 am on April 17, 2025:

    I’d either delete this now or put it before the actual closing. And do we really need to make this unlikely scenario occupy so much space? And in the other case we simply used LogError instead:

    0// Make sure that the file is closed before we call `FlushUndoFile`.
1if (file.fclose()) {
2    LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
3    return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
4}
    vasild commented at 2:31 pm on May 2, 2025:
    Moved the comment before the actual closing. Reduced as well.
  111. in src/node/blockstorage.cpp:1169 in 1f71111e21 outdated 
    1164@@ -1142,6 +1165,11 @@ static auto InitBlocksdirXorKey(const BlockManager::Options& opts)
1165 #endif
1166         )};
1167         xor_key_file << xor_key;
1168+        if (xor_key_file.fclose() != 0) {
1169+            throw std::runtime_error{strprintf("Error closing XOR key file %s: %s\n",
    l0rinc commented at 11:34 am on April 17, 2025:

    nit:

    0            throw std::runtime_error{strprintf("Error closing XOR key file %s: %s",
    vasild commented at 2:31 pm on May 2, 2025:
    Done.
  112. vasild force-pushed on Apr 17, 2025
  113. vasild commented at 11:36 am on April 17, 2025: contributor
    1f71111e21...c52b673bf8: #29307 (review)
  114. in src/test/fuzz/autofile.cpp:68 in c52b673bf8 outdated 
    61@@ -62,5 +62,10 @@ FUZZ_TARGET(autofile)
62         if (f != nullptr) {
63             fclose(f);
64         }
65+    } else {
66+        // FuzzedFileProvider::close() is expected to fail sometimes. Don't let
67+        // the destructor of AutoFile be upset by a failing fclose(). Close it
68+        // explicitly (and ignore any errors) so that the destructor is a noop.
    l0rinc commented at 11:40 am on April 17, 2025:
    why do we want the destructor to be a noop?
    vasild commented at 2:33 pm on May 2, 2025:
    Removed this outdated comment.
  115. in src/test/streams_tests.cpp:572 in c52b673bf8 outdated 
    567@@ -566,7 +568,9 @@ BOOST_AUTO_TEST_CASE(buffered_reader_matches_autofile_random_content)
568 
569     // Write out the file with random content
570     {
571-        AutoFile{test_file.Open(pos, /*read_only=*/false), obfuscation}.write(m_rng.randbytes<std::byte>(file_size));
572+        AutoFile f{test_file.Open(pos, /*read_only=*/false), obfuscation};
573+        f.write(m_rng.randbytes<std::byte>(file_size));
574+        BOOST_REQUIRE_EQUAL(f.fclose(), 0);
    l0rinc commented at 11:41 am on April 17, 2025:
    nit: this likely isn’t strictly necessary, as long as it contains all the data (checked below), it should work correctly - but it’s fine either way
    vasild commented at 2:34 pm on May 2, 2025:
    I think it is better to stop the test right away if the closing fails.
  116. in src/streams.h:406 in c52b673bf8 outdated 
    402 {
403 protected:
404     std::FILE* m_file;
405     std::vector<std::byte> m_xor;
406     std::optional<int64_t> m_position;
407+    bool m_was_written{false};
    l0rinc commented at 11:51 am on April 17, 2025:
    Can you separate the AutoFile changes to a new commit, explaining these changes separately from the changes that are calling close explicitly?
    vasild commented at 2:35 pm on May 2, 2025:
    Done.
  117. in src/streams.h:423 in c52b673bf8 outdated 
    420+            // corrupted and must be handled properly at the call site.
421+            Assume(IsNull());
422+        }
423+
424+        if (fclose() != 0) {
425+            LogPrintLevel(BCLog::ALL, BCLog::Level::Error, "Failed to close file: %s", SysErrorString(errno));
    l0rinc commented at 11:53 am on April 17, 2025: 
    0            LogError("Failed to close file: %s", SysErrorString(errno));
    vasild commented at 2:35 pm on May 2, 2025:
    Done.
  118. in src/streams.h:435 in c52b673bf8 outdated 
    431     AutoFile& operator=(const AutoFile&) = delete;
432 
433     bool feof() const { return std::feof(m_file); }
434 
435-    int fclose()
436+    [[nodiscard]] int fclose()
    l0rinc commented at 11:54 am on April 17, 2025:
    👍 for [[nodiscard]], but this seems like the perfect opportunity to unify it with how e.g. feof works and change fclose to return a boolean instead - since we don’t seem to check other values than 0.
    vasild commented at 2:40 pm on May 2, 2025:

    Hmm, the OS feof() returns nonzero if end-of-file. This makes it possible to use the natural:

    0if (feof()) { // end-of-file has been reached

    But fclose() returns zero if the file has been closed. I do not want to invert the return value because that would be confusing for people that know the interface of the OS fclose(). Or even more confusing, if the return is not inverted and converted to bool:

    0bool fclose(...);
1
2if (fclose()) { // error!
    l0rinc commented at 5:23 pm on May 2, 2025:
    I find returning error codes for boolean values a lot more confusing. Edit: And we’re already doing if (!file.Commit()) {, I find this a lot more intuitive than fake error codes.
    hodlinator commented at 12:51 pm on May 3, 2025:
    In that case it would be better to call it bool AutoFile::Close() rather than bool AutoFile::fclose().
    l0rinc commented at 1:27 pm on May 3, 2025:

    Agree, it’s what I wrote in the review as well:

    the current fclose returns a fake error code, but is actually a boolean - may make sense to rename it (e.g. Close, similarly to how we’ve wrapped ftruncate with TruncateFile) and migrate to returning a boolean to avoid the current confusion.

    vasild commented at 12:46 pm on May 5, 2025:

    The interface of AutoFile::fclose() is not changed by this PR and is not the purpose of this PR to change it. Thus I will leave it as it is.

    I think that the interface of the current AutoFile::fclose() is just fine. It mimics 1:1 the libc fclose(3) which is in line with the majority of the other system functions which:

    return the value 0 if successful; otherwise the value -1 is returned and the global variable errno is set to indicate the error

    I don’t think that is “fake” and I do not know why it would be confusing. It is well established. The benefit of mimicking 1:1 is that there is zero learning curve for people that read the Bitcoin Core code and are familiar with the libc functions.

  119. in src/streams.h:417 in c52b673bf8 outdated 
    414+    {
415+        if (m_was_written) {
416+            // Callers that wrote to the file must have closed it explicitly
417+            // with the fclose() method and checked that the close succeeded.
418+            // This is because here from the destructor we have no way to signal
419+            // error due to close which, after write, could mean the file is
    l0rinc commented at 11:55 am on April 17, 2025:
    not sure what these lines mean
    vasild commented at 2:41 pm on May 2, 2025:

    Elaborated a little bit:

    0            // Callers that wrote to the file must have closed it explicitly
1            // with the fclose() method and checked that the close succeeded.
2            // This is because here from the destructor we have no way to signal
3            // error due to close which, after write, could mean the file is
4            // corrupted and must be handled properly at the call site.
5            // Destructors in C++ cannot signal an error to the callers because
6            // they do not return a value and are not allowed to throw exceptions.
    l0rinc commented at 6:20 pm on May 2, 2025:

    because here from the destructor

    Still not sure what this means exactly, would this maybe work better?

    0            // This is because here in the destructor we have no way to signal
1            // errors from close(), which, after write, could mean the file is
    vasild commented at 12:53 pm on May 5, 2025:
    Done
  120. in src/test/flatfile_tests.cpp:84 in c52b673bf8 outdated 
    80@@ -79,13 +81,15 @@ BOOST_AUTO_TEST_CASE(flatfile_open)
81 
82         file >> LIMITED_STRING(text, 256);
83         BOOST_CHECK_EQUAL(text, line2);
84+        BOOST_REQUIRE_EQUAL(file.fclose(), 0);
    l0rinc commented at 11:56 am on April 17, 2025:
    ‘require’ seems a bit harsh, but shouldn’t happen often
    vasild commented at 2:41 pm on May 2, 2025:
    I think it is better to stop the test immediately if closing a file gives an error.
  121. in src/streams.cpp:98 in c52b673bf8 outdated 
    94@@ -95,6 +95,7 @@ void AutoFile::write(std::span<const std::byte> src)
95             src = src.subspan(buf_now.size());
96         }
97     }
98+    m_was_written = true;
    l0rinc commented at 11:58 am on April 17, 2025:
    should this be added to AutoFile::write_buffer as well?
    vasild commented at 2:42 pm on May 2, 2025:
    What to be added to AutoFile::write_buffer?
    l0rinc commented at 5:22 pm on May 2, 2025:
    m_was_written = true, since AutoFile::write_buffer also dirties (which is the term we use in other cases, if you think it describes the behavior better, consider renaming)
    vasild commented at 12:54 pm on May 5, 2025:
    I see. Done. I also moved the m_was_written = true to be just after the fwrite() calls.
  122. l0rinc changes_requested
  123. l0rinc commented at 12:14 pm on April 17, 2025: contributor

    Concept ACK, we definitely want to know if this happens!

    • It seems we’re using the return value as a boolean - can make make it return one?
    • The error messages seem all over the place, can we unify them to LogError?
    • Not sure if scope-ing the AutoFile + closing it explicitly is the best solution - would probably prefer @ryanofsky’s std::function<void()> on_error or @maflcko’s recommendation to let AutoFile guarantee proper closing.
    • Sometimes we’re explicitly showing a closing related failure, other times we’re reusing previous error messages - it’s not a very important or likely scenario, we could likely add it to other erroneous cases.
    • We have discernible changes here, all in a single commit - would prefer separating the concerns a bit more with better commit message explanations
  124. vasild force-pushed on May 2, 2025
  125. vasild commented at 2:27 pm on May 2, 2025: contributor
    c52b673bf8...c9cd7d3aa9: rebase and address suggestions
  126. vasild force-pushed on May 2, 2025
  127. vasild commented at 2:34 pm on May 2, 2025: contributor
    c9cd7d3aa9...66087035cf: remove outdated comment
  128. vasild force-pushed on May 2, 2025
  129. vasild commented at 2:48 pm on May 2, 2025: contributor
    66087035cf...fe2d1955f6: add missing includes in the first commit
  130. l0rinc changes_requested
  131. l0rinc commented at 6:33 pm on May 2, 2025: contributor

    Code review fe2d1955f6333c57f54aed2a2a8b3cdaa6bf9d71

    • blocker: the new AutoFile::write_buffer should also mark the file as dirty.
    • the current fclose returns a fake error code, but is actually a boolean - may make sense to rename it (e.g. Close, similarly to how we’ve wrapped ftruncate with TruncateFile) and migrate to returning a boolean to avoid the current confusion.
      0diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
  1index 9e6ea8d7d0..6b47a0fa0a 100644
  2--- a/src/index/blockfilterindex.cpp
  3+++ b/src/index/blockfilterindex.cpp
  4@@ -13,6 +13,7 @@
  5 #include <node/blockstorage.h>
  6 #include <undo.h>
  7 #include <util/fs_helpers.h>
  8+#include <util/syserror.h>
  9 #include <validation.h>
 10 
 11 /* The index database stores three items for each block: the disk location of the encoded filter,
 12@@ -151,8 +152,13 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
 13         LogError("%s: Failed to open filter file %d\n", __func__, pos.nFile);
 14         return false;
 15     }
 16-    if (!file.Commit() || file.fclose() != 0) {
 17+    if (!file.Commit()) {
 18         LogError("%s: Failed to commit filter file %d\n", __func__, pos.nFile);
 19+        (void)file.fclose();
 20+        return false;
 21+    }
 22+    if (file.fclose() != 0) {
 23+        LogError("Failed to close filter file %d after commit: %s", pos.nFile, SysErrorString(errno));
 24         return false;
 25     }
 26 
 27@@ -205,8 +211,13 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
 28             LogPrintf("%s: Failed to truncate filter file %d\n", __func__, pos.nFile);
 29             return 0;
 30         }
 31-        if (!last_file.Commit() || last_file.fclose() != 0) {
 32+        if (!last_file.Commit()) {
 33             LogPrintf("%s: Failed to commit filter file %d\n", __func__, pos.nFile);
 34+            (void)last_file.fclose();
 35+            return 0;
 36+        }
 37+        if (last_file.fclose() != 0) {
 38+            LogError("Failed to close filter file %d after commit: %s", pos.nFile, SysErrorString(errno));
 39             return 0;
 40         }
 41 
 42@@ -231,7 +242,7 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
 43     fileout << filter.GetBlockHash() << filter.GetEncodedFilter();
 44 
 45     if (fileout.fclose() != 0) {
 46-        LogPrintf("Failed to close filter file %d: %s", pos.nFile, SysErrorString(errno));
 47+        LogError("Failed to close filter file %d: %s", pos.nFile, SysErrorString(errno));
 48         return 0;
 49     }
 50 
 51diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
 52index 8b21356c42..e6a979b5f3 100644
 53--- a/src/node/blockstorage.cpp
 54+++ b/src/node/blockstorage.cpp
 55@@ -33,6 +33,7 @@
 56 #include <util/fs.h>
 57 #include <util/signalinterrupt.h>
 58 #include <util/strencodings.h>
 59+#include <util/syserror.h>
 60 #include <util/translation.h>
 61 #include <validation.h>
 62 
 63@@ -940,37 +941,31 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
 64             return false;
 65         }
 66 
 67+        // Open history file to append
 68+        AutoFile file{OpenUndoFile(pos)};
 69+        if (file.IsNull()) {
 70+            LogError("OpenUndoFile failed for %s while writing block undo", pos.ToString());
 71+            return FatalError(m_opts.notifications, state, _("Failed to write undo data."));
 72+        }
 73         {
 74-            // Open history file to append
 75-            AutoFile file{OpenUndoFile(pos)};
 76-            if (file.IsNull()) {
 77-                LogError("OpenUndoFile failed for %s while writing block undo", pos.ToString());
 78-                return FatalError(m_opts.notifications, state, _("Failed to write undo data."));
 79-            }
 80+            BufferedWriter fileout{file};
 81+
 82+            // Write index header
 83+            fileout << GetParams().MessageStart() << blockundo_size;
 84+            pos.nPos += STORAGE_HEADER_BYTES;
 85             {
 86-                BufferedWriter fileout{file};
 87-
 88-                // Write index header
 89-                fileout << GetParams().MessageStart() << blockundo_size;
 90-                pos.nPos += STORAGE_HEADER_BYTES;
 91-                {
 92-                    // Calculate checksum
 93-                    HashWriter hasher{};
 94-                    hasher << block.pprev->GetBlockHash() << blockundo;
 95-                    // Write undo data & checksum
 96-                    fileout << blockundo << hasher.GetHash();
 97-                }
 98+                // Calculate checksum
 99+                HashWriter hasher{};
100+                hasher << block.pprev->GetBlockHash() << blockundo;
101+                // Write undo data & checksum
102+                fileout << blockundo << hasher.GetHash();
103             }
104+        }
105 
106-            if (file.fclose() != 0) {
107-                LogPrintLevel(BCLog::BLOCKSTORAGE,
108-                              BCLog::Level::Error,
109-                              "Failed to close block undo file %s: %s",
110-                              pos.ToString(),
111-                              SysErrorString(errno));
112-                return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
113-            }
114-            // Make sure that the file is closed before we call `FlushUndoFile`.
115+        // Make sure that the file is closed before we call `FlushUndoFile`.
116+        if (file.fclose() != 0) {
117+            LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
118+            return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
119         }
120 
121         // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order)
122@@ -1114,11 +1109,7 @@ FlatFilePos BlockManager::WriteBlock(const CBlock& block, int nHeight)
123     }
124 
125     if (file.fclose() != 0) {
126-        LogPrintLevel(BCLog::BLOCKSTORAGE,
127-                      BCLog::Level::Error,
128-                      "Failed to close block file %s: %s",
129-                      pos.ToString(),
130-                      SysErrorString(errno));
131+        LogError("Failed to close block file %s: %s", pos.ToString(), SysErrorString(errno));
132         m_opts.notifications.fatalError(_("Failed to close file when writing block."));
133         return FlatFilePos();
134     }
135@@ -1166,7 +1157,7 @@ static auto InitBlocksdirXorKey(const BlockManager::Options& opts)
136         )};
137         xor_key_file << xor_key;
138         if (xor_key_file.fclose() != 0) {
139-            throw std::runtime_error{strprintf("Error closing XOR key file %s: %s\n",
140+            throw std::runtime_error{strprintf("Error closing XOR key file %s: %s",
141                                                fs::PathToString(xor_key_path),
142                                                SysErrorString(errno))};
143         }
144diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
145index ea763efc98..3a59930c4b 100644
146--- a/src/policy/fees.cpp
147+++ b/src/policy/fees.cpp
148@@ -19,6 +19,7 @@
149 #include <uint256.h>
150 #include <util/fs.h>
151 #include <util/serfloat.h>
152+#include <util/syserror.h>
153 #include <util/time.h>
154 
155 #include <algorithm>
156@@ -953,11 +954,16 @@ void CBlockPolicyEstimator::Flush() {
157 void CBlockPolicyEstimator::FlushFeeEstimates()
158 {
159     AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")};
160-    if (est_file.IsNull() || !Write(est_file) || est_file.fclose() != 0) {
161-        LogWarning("Failed to write fee estimates to %s. Continuing anyway.", fs::PathToString(m_estimation_filepath));
162-    } else {
163-        LogPrintf("Flushed fee estimates to %s.\n", fs::PathToString(m_estimation_filepath.filename()));
164+    if (est_file.IsNull() || !Write(est_file)) {
165+        LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
166+        (void)est_file.fclose();
167+        return;
168+    }
169+    if (est_file.fclose() != 0) {
170+        LogError("Failed to close fee estimates file %s: %s. Continuing anyway.", fs::PathToString(m_estimation_filepath), SysErrorString(errno));
171+        return;
172     }
173+    LogPrintf("Flushed fee estimates to %s.\n", fs::PathToString(m_estimation_filepath.filename()));
174 }
175 
176 bool CBlockPolicyEstimator::Write(AutoFile& fileout) const
177diff --git a/src/streams.h b/src/streams.h
178index c13cebc5c6..09b2e20c08 100644
179--- a/src/streams.h
180+++ b/src/streams.h
181@@ -416,11 +416,13 @@ public:
182             // This is because here from the destructor we have no way to signal
183             // error due to close which, after write, could mean the file is
184             // corrupted and must be handled properly at the call site.
185+            // Destructors in C++ cannot signal an error to the callers because
186+            // they do not return a value and are not allowed to throw exceptions.
187             Assume(IsNull());
188         }
189 
190         if (fclose() != 0) {
191-            LogPrintLevel(BCLog::ALL, BCLog::Level::Error, "Failed to close file: %s", SysErrorString(errno));
192+            LogError("Failed to close file: %s", SysErrorString(errno));
193         }
194     }
195 
196diff --git a/src/test/fuzz/autofile.cpp b/src/test/fuzz/autofile.cpp
197index cdbae9c3cc..8d17624da6 100644
198--- a/src/test/fuzz/autofile.cpp
199+++ b/src/test/fuzz/autofile.cpp
200@@ -63,9 +63,6 @@ FUZZ_TARGET(autofile)
201             fclose(f);
202         }
203     } else {
204-        // FuzzedFileProvider::close() is expected to fail sometimes. Don't let
205-        // the destructor of AutoFile be upset by a failing fclose(). Close it
206-        // explicitly (and ignore any errors) so that the destructor is a noop.
207         (void)auto_file.fclose();
208     }
209 }
  132. in src/node/blockstorage.cpp:970 in fe2d1955f6 outdated 
    964 
965-            fileout.flush(); // Make sure `AutoFile`/`BufferedWriter` go out of scope before we call `FlushUndoFile`
966+        // Make sure that the file is closed before we call `FlushUndoFile`.
967+        if (file.fclose() != 0) {
968+            LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
969+            return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
    l0rinc commented at 6:36 pm on May 2, 2025:
    do we need both messages? How will this be displayed in the logs, isn’t it redundant?
    vasild commented at 12:32 pm on May 5, 2025:

    This just mimics the error handling when the file cannot be opened:

    https://github.com/bitcoin/bitcoin/blob/53ccb75f0c47e1e48504cb392d7589548a8d25a2/src/node/blockstorage.cpp#L945-L949

  133. vasild force-pushed on May 5, 2025
  134. vasild commented at 12:39 pm on May 5, 2025: contributor

    fe2d1955f6...2c1a8ff0c0: address suggestions

    AutoFile::write_buffer should also mark the file as dirty

    Done.

    the current fclose returns a fake error code…

    Replied to the discussion thread at #29307 (review)

  135. vasild force-pushed on May 5, 2025
  136. DrahtBot added the label CI failed on May 5, 2025
  137. DrahtBot commented at 1:03 pm on May 5, 2025: contributor

    🚧 At least one of the CI tasks failed. Task MSan, depends: https://github.com/bitcoin/bitcoin/runs/41649198396 LLM reason (✨ experimental): The CI failure was caused by an assertion failure in streams.h (AutoFile destructor) during the execution of streams_tests.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  138. vasild commented at 1:04 pm on May 5, 2025: contributor
    2c1a8ff0c0...61ba3fc494: fix CI
  139. l0rinc commented at 1:31 pm on May 5, 2025: contributor

    write_buffer was also marked as dirty in latest push.

    untested ACK 61ba3fc494f2f5e60b668316c84cecbdc123e5fa

    Q: shouldn’t Truncate and Commit also set m_was_written = true?

  140. DrahtBot requested review from ryanofsky on May 5, 2025
  141. DrahtBot requested review from maflcko on May 5, 2025
  142. DrahtBot requested review from hodlinator on May 5, 2025
  143. DrahtBot removed the label CI failed on May 5, 2025
  144. vasild force-pushed on May 5, 2025
  145. vasild commented at 2:37 pm on May 5, 2025: contributor

    61ba3fc494...9057ab8b80:

    shouldn’t Truncate and Commit also set m_was_written = true?

    Good question! I think Truncate() - yes because it changes the contents of the file. Changed.

    Commit() does not, so I left it alone. In other words - if a file is opened, none of the write(), write_buffer() or Truncate() is called, Commit() is called, and the file object is destroyed, could that be a problem? I think no because the contents of the file was not changed.

  146. in src/node/blockstorage.cpp:965 in 9057ab8b80 outdated 
    959@@ -959,8 +960,12 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
960                 // Write undo data & checksum
961                 fileout << blockundo << hasher.GetHash();
962             }
963+        }
    hodlinator commented at 9:10 am on May 20, 2025:

    nit: Could leave comment justifying scope:

    0        } // Make sure BufferedWriter flushes pending data to file.
    vasild commented at 11:01 am on May 22, 2025:
    Done.
  147. in src/node/blockstorage.cpp:973 in 9057ab8b80 outdated 
    967+        if (file.fclose() != 0) {
968+            LogError("Failed to close block undo file %s: %s", pos.ToString(), SysErrorString(errno));
969+            return FatalError(m_opts.notifications, state, _("Failed to close block undo file."));
970         }
971 
972         // rev files are written in block height order, whereas blk files are written as blocks come in (often out of order)
    hodlinator commented at 9:14 am on May 20, 2025:
    What do you think about flatfile.cpp? It has a bunch of unchecked fcloses.
    vasild commented at 11:01 am on May 22, 2025:
    Yes. Added one more commit to handle those.
  148. in src/node/mempool_persist.cpp:206 in 9057ab8b80 outdated 
    202@@ -201,7 +203,10 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
203 
204         if (!skip_file_commit && !file.Commit())
205             throw std::runtime_error("Commit failed");
    hodlinator commented at 9:20 am on May 20, 2025:

    This throw would trigger the Assume() in ~AutoFile() to fail, no?

    Instead of needing to comb through each line in this change and in future modifications after this PR, I think calling a lambda from the destructor on failure is marginally safer, as suggested in #29307#pullrequestreview-2288411339 (but with more correct errno handling + maybe requiring the lambda be noexcept).

    Requiring a lambda for the destructor should at the very least be listed as an alternative in the PR description with your rationale against it.

    vasild commented at 11:14 am on May 22, 2025:

    This throw would trigger the Assume() in ~AutoFile() to fail, no?

    Yes. Added a fclose() call before the throw to address that.

    I will consider the lambda in the destructor again. From the fclose() callers, I counted:

    • 7 that ignore the fclose() return value
    • 7 that log message + take a different code path
    • 4 throw

    Taking a different code path upon fclose() failure would be tricky when that is called from the AutoFile destructor. It would require code like:

    0AutoFile a;
1... possibly a lot of code ...
2if (a.fclose() != 0) {
3  LogError();
4  return false;
5}
6return true;

    to be written like:

    0bool fclose_failed{false};
1{
2    AutoFile a{..., [&fclose_failed] () { fclose_failed = true; }};
3    ... possibly a lot of code, all of it now indented further ...
4}
5if (fclose_failed) {
6    LogError();
7    return false;
8}
9return true;

    I like the idea that the caller cannot easily forget to handle a fclose() failure though.

    hodlinator commented at 8:49 pm on May 27, 2025:

    In this specific case of mempool_persist.cpp/DumpMempool my variant simply had:

    0    FileWriter file{mockable_fopen_function(file_fspath, "wb"), [] (int err) {
1        Assume(std::uncaught_exceptions() > 0); // Only expected when exception is thrown before fclose() below.
2    }};

    Which would serve to enforce that we either already explicitly fclose()d the file and checked errno further down in the function (leading to the lambda not being called upon destruction), or that an exception was triggered before reaching that far (in which case the Assume() in the lambda succeeds). (This assumes that the thrown exception is worse than the failure to close the file, which may not always be true).

    For other cases I ended up with scopes and extra variables to check for failures, like in your examples in the comment above. Agree that it would be nice to find something more elegant… and until we do, your current approach in this PR seems okay.

    nit: I still think the user-provided lambda in destructor approach merits an alternative 4 in the PR description.

    vasild commented at 3:07 pm on June 11, 2025:

    I will consider the lambda in the destructor again

    I am still not excited about that. Added “alternative 4 in the PR description” with my thoughts.

  149. in src/test/fuzz/autofile.cpp:50 in 9057ab8b80 outdated 
    46@@ -47,7 +47,7 @@ FUZZ_TARGET(autofile)
47                 }
48             },
49             [&] {
50-                auto_file.fclose();
51+                (void)auto_file.fclose();
    hodlinator commented at 9:30 am on May 20, 2025:

    Why not use this pattern in all fuzz-tests?

    0                assert(auto_file.fclose() == 0);
    l0rinc commented at 1:38 pm on May 20, 2025:
    Is this different from your previous suggestion in #29307 (review)?
    hodlinator commented at 9:06 pm on May 20, 2025:
    Not really, seems like it was changed in that specific file and resolved without being changed more broadly.
    vasild commented at 10:12 am on May 22, 2025:
    Fuzzed FILE*, which comes from fuzzed_file_provider.open() is expected to have a failing fclose(). Should not fail the test for that, I replied there: #29307 (review)
  150. hodlinator commented at 11:45 am on May 20, 2025: contributor

    Code review 9057ab8b80137a89df68a3ed4bc49f9926e0f634

    I like how the AutoFile change now is broken out into a 2nd commit.

  151. vasild force-pushed on May 22, 2025
  152. vasild commented at 11:00 am on May 22, 2025: contributor
    9057ab8b80...c7b68dceb2: address suggestions
  153. DrahtBot added the label CI failed on May 22, 2025
  154. in src/flatfile.cpp:49 in c7b68dceb2 outdated 
    45@@ -46,7 +46,9 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only) const
46     }
47     if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {
48         LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path));
49-        fclose(file);
50+        if (fclose(file) == -1) {
    hodlinator commented at 12:42 pm on May 22, 2025:

    Probably safer to check against 0 in this file as you’ve done elsewhere in this PR:

    0        if (fclose(file) != 0) {

    (Seems like we can be more certain of the value of 0/success than EOF/failure: https://www.man7.org/linux/man-pages/man3/fclose.3.html https://en.cppreference.com/w/c/io/fclose)

    vasild commented at 3:09 pm on June 11, 2025:
    Done.
  155. DrahtBot removed the label CI failed on May 22, 2025
  156. in src/flatfile.cpp:75 in c7b68dceb2 outdated 
    69@@ -68,7 +70,10 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_
70             if (file) {
71                 LogDebug(BCLog::VALIDATION, "Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile);
72                 AllocateFileRange(file, pos.nPos, inc_size);
73-                fclose(file);
74+                if (fclose(file) == -1) {
75+                    LogError("Cannot close file %s%05u.dat after extending it with %u bytes", m_prefix, pos.nFile, new_size);
76+                    return 0;
    hodlinator commented at 8:22 pm on June 2, 2025:

    Not sure how to handle this in the callers, probably something along these lines:

     0diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
 1index 6b47a0fa0a..332e700995 100644
 2--- a/src/index/blockfilterindex.cpp
 3+++ b/src/index/blockfilterindex.cpp
 4@@ -227,10 +227,13 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&
 5 
 6     // Pre-allocate sufficient space for filter data.
 7     bool out_of_space;
 8-    m_filter_fileseq->Allocate(pos, data_size, out_of_space);
 9+    const size_t allocated_size = m_filter_fileseq->Allocate(pos, data_size, out_of_space);
10     if (out_of_space) {
11         LogPrintf("%s: out of disk space\n", __func__);
12         return 0;
13+    } else if (allocated_size < data_size) {
14+        LogError("Failed to allocate %u for file %s.", data_size, pos.ToString());
15+        return 0;
16     }
17 
18     AutoFile fileout{m_filter_fileseq->Open(pos)};
19diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
20index a344e13c89..3ff3aa9122 100644
21--- a/src/node/blockstorage.cpp
22+++ b/src/node/blockstorage.cpp
23@@ -873,6 +873,9 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n
24     if (out_of_space) {
25         m_opts.notifications.fatalError(_("Disk space is too low!"));
26         return {};
27+    } else if (bytes_allocated < nAddSize) {
28+        m_opts.notifications.fatalError(_("Failed allocating disk space!"));
29+        return {};
30     }
31     if (bytes_allocated != 0 && IsPruneMode()) {
32         m_check_for_pruning = true;
33@@ -918,6 +921,8 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
34     size_t bytes_allocated = m_undo_file_seq.Allocate(pos, nAddSize, out_of_space);
35     if (out_of_space) {
36         return FatalError(m_opts.notifications, state, _("Disk space is too low!"));
37+    } else if (bytes_allocated < nAddSize) {
38+        return FatalError(m_opts.notifications, state, _("Failed allocating disk space!"));
39     }
40     if (bytes_allocated != 0 && IsPruneMode()) {
41         m_check_for_pruning = true;
    vasild commented at 3:13 pm on June 11, 2025:
    I agree that callers of Allocate() can do better error handling. That seems to be out of the scope of this PR because this PR does not change the semantic of Allocate(). Also in master it could return 0 and have out_of_space == false. That means an error has occurred, e.g. it would do that if it cannot open the file (in master). Now it will also do it if it cannot close the file.
  157. hodlinator commented at 8:29 pm on June 2, 2025: contributor

    Code review c7b68dceb29ef67796f7edf79a161bbda766169c

    This is a clear incremental improvement over what is on master.

    Thanks for taking my suggestions!

    Don’t foresee any further issues preventing an A-C-K beyond the inline comments below.

  158. luke-jr referenced this in commit db608792d0 on Jun 5, 2025
  159. luke-jr referenced this in commit b395f66847 on Jun 5, 2025
  160. luke-jr referenced this in commit 8b2da0a368 on Jun 5, 2025
  161. luke-jr referenced this in commit e8313c9b50 on Jun 5, 2025
  162. vasild force-pushed on Jun 11, 2025
  163. vasild commented at 3:09 pm on June 11, 2025: contributor
    c7b68dceb2...004ec6968e: rebase and address suggestion
  164. DrahtBot added the label Needs rebase on Jun 12, 2025
  165. vasild force-pushed on Jun 13, 2025
  166. vasild commented at 12:29 pm on June 13, 2025: contributor
    004ec6968e...f8a7580a08: rebase due to conflicts
  167. DrahtBot removed the label Needs rebase on Jun 13, 2025
  168. in src/test/util/chainstate.h:51 in 75b49d469b outdated 
    47@@ -48,7 +48,7 @@ CreateAndActivateUTXOSnapshot(
48     AutoFile auto_outfile{outfile};
49 
50     UniValue result = CreateUTXOSnapshot(
51-        node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path);
52+        node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); // Will close auto_outfile.
    hodlinator commented at 12:53 pm on June 13, 2025:
    nit in 75b49d469b2a993dbcd64057b008f247f79155cd: Could avoid touching this file in this commit, and maybe move 727d4793747497f9be706b5f6dffb409508fe223 to happen as the first commit in the PR?
    vasild commented at 1:36 pm on June 16, 2025:
    Done
  169. hodlinator approved
  170. hodlinator commented at 1:02 pm on June 13, 2025: contributor

    ACK f8a7580a0816586f75ba9b0e79300c893d362f9f

    Mainly changes AutoFile to encourage judicious error checking upon file closure, surfacing failures to complete writing files to disk.

    Several alternatives have been considered (listed in PR description) and I agree this is probably the best option for now.

  171. DrahtBot requested review from l0rinc on Jun 13, 2025
  172. in src/addrdb.cpp:85 in 75b49d469b outdated 
    84     }
85-    fileout.fclose();
86+    if (fileout.fclose() != 0) {
87+        const int errno_save{errno};
88+        remove(pathTmp);
89+        LogError("Failed to close file %s: %s", fs::PathToString(pathTmp), SysErrorString(errno_save));
    l0rinc commented at 3:38 pm on June 13, 2025:

    In other cased you’ve added after commit to clarify when closing failed.

    Also previous errors we’ve prefixed with __func__:

    0        LogError("s: Failed to close file %s after commit: %s", __func__, fs::PathToString(pathTmp), SysErrorString(errno_save));

    Looking at the other examples you’ve changed, I assume this was deliberate - I’d still prefer unifying them, but I don’t mind tackling it in a separate PR either.

    vasild commented at 1:36 pm on June 16, 2025:

    Added “after commit”.

    __func__ is redundant with -logsourcelocations: #29307 (review) #29307 (review)

    l0rinc commented at 1:51 pm on June 16, 2025:

    func is redundant

    Yes, would be great to unify the affected messages - either by adding or removing in every touched method. But we can of course do it in a separate PR as well.

  173. in src/test/streams_tests.cpp:383 in 75b49d469b outdated 
    379@@ -379,8 +380,8 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
380     // by the rewind window (relative to our farthest read position, 40).
381     BOOST_CHECK(bf.GetPos() <= 30U);
382 
383-    // We can explicitly close the file, or the destructor will do it.
384-    file.fclose();
385+    // Explicitly close the file and check that the close succeeds.
    l0rinc commented at 3:45 pm on June 13, 2025:
    comments isn’t useful anymore, the code already states the same
    vasild commented at 1:34 pm on June 16, 2025:
    Removed.
  174. l0rinc approved
  175. l0rinc commented at 3:49 pm on June 13, 2025: contributor

    ACK f8a7580a0816586f75ba9b0e79300c893d362f9f

    Changes since last review:

    • adjust AutoFile parameter move semantics;
    • log unsuccessful closes as a last attempt;
    • add m_was_written to Truncate;
    • & explanatory comments.
      0diff --git a/src/flatfile.cpp b/src/flatfile.cpp
  1index 388b30efae..df6596e940 100644
  2--- a/src/flatfile.cpp
  3+++ b/src/flatfile.cpp
  4@@ -46,7 +46,9 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only) const
  5     }
  6     if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {
  7         LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path));
  8-        fclose(file);
  9+        if (fclose(file) != 0) {
 10+            LogError("Unable to close file %s", fs::PathToString(path));
 11+        }
 12         return nullptr;
 13     }
 14     return file;
 15@@ -68,7 +70,10 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_
 16             if (file) {
 17                 LogDebug(BCLog::VALIDATION, "Pre-allocating up to position 0x%x in %s%05u.dat\n", new_size, m_prefix, pos.nFile);
 18                 AllocateFileRange(file, pos.nPos, inc_size);
 19-                fclose(file);
 20+                if (fclose(file) != 0) {
 21+                    LogError("Cannot close file %s%05u.dat after extending it with %u bytes", m_prefix, pos.nFile, new_size);
 22+                    return 0;
 23+                }
 24                 return inc_size;
 25             }
 26         } else {
 27@@ -86,17 +91,24 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) const
 28         return false;
 29     }
 30     if (finalize && !TruncateFile(file, pos.nPos)) {
 31-        fclose(file);
 32         LogError("%s: failed to truncate file %d\n", __func__, pos.nFile);
 33+        if (fclose(file) != 0) {
 34+            LogError("Failed to close file %d", pos.nFile);
 35+        }
 36         return false;
 37     }
 38     if (!FileCommit(file)) {
 39-        fclose(file);
 40         LogError("%s: failed to commit file %d\n", __func__, pos.nFile);
 41+        if (fclose(file) != 0) {
 42+            LogError("Failed to close file %d", pos.nFile);
 43+        }
 44         return false;
 45     }
 46     DirectoryCommit(m_dir);
 47 
 48-    fclose(file);
 49+    if (fclose(file) != 0) {
 50+        LogError("Failed to close file %d after flush", pos.nFile);
 51+        return false;
 52+    }
 53     return true;
 54 }
 55diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
 56index 557ddaceab..81f42827d9 100644
 57--- a/src/node/blockstorage.cpp
 58+++ b/src/node/blockstorage.cpp
 59@@ -961,6 +961,7 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
 60                 // Write undo data & checksum
 61                 fileout << blockundo << hasher.GetHash();
 62             }
 63+            // BufferedWriter will flush pending data to file when fileout goes out of scope.
 64         }
 65 
 66         // Make sure that the file is closed before we call `FlushUndoFile`.
 67diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp
 68index 2551a43166..d4d9263973 100644
 69--- a/src/node/mempool_persist.cpp
 70+++ b/src/node/mempool_persist.cpp
 71@@ -201,8 +201,10 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock
 72         LogInfo("Writing %d unbroadcast transactions to file.\n", unbroadcast_txids.size());
 73         file << unbroadcast_txids;
 74 
 75-        if (!skip_file_commit && !file.Commit())
 76+        if (!skip_file_commit && !file.Commit()) {
 77+            (void)file.fclose();
 78             throw std::runtime_error("Commit failed");
 79+        }
 80         if (file.fclose() != 0) {
 81             throw std::runtime_error(
 82                 strprintf("Error closing %s: %s", fs::PathToString(file_fspath), SysErrorString(errno)));
 83diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 84index 4f5a6b8434..be953cefde 100644
 85--- a/src/rpc/blockchain.cpp
 86+++ b/src/rpc/blockchain.cpp
 87@@ -83,7 +83,7 @@ UniValue WriteUTXOSnapshot(
 88     CCoinsViewCursor* pcursor,
 89     CCoinsStats* maybe_stats,
 90     const CBlockIndex* tip,
 91-    AutoFile& afile,
 92+    AutoFile&& afile,
 93     const fs::path& path,
 94     const fs::path& temppath,
 95     const std::function<void()>& interruption_point = {});
 96@@ -3115,7 +3115,14 @@ static RPCHelpMan dumptxoutset()
 97         }
 98     }
 99 
100-    UniValue result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point);
101+    UniValue result = WriteUTXOSnapshot(*chainstate,
102+                                        cursor.get(),
103+                                        &stats,
104+                                        tip,
105+                                        std::move(afile),
106+                                        path,
107+                                        temppath,
108+                                        node.rpc_interruption_point);
109     fs::rename(temppath, path);
110 
111     result.pushKV("path", path.utf8string());
112@@ -3167,7 +3174,7 @@ UniValue WriteUTXOSnapshot(
113     CCoinsViewCursor* pcursor,
114     CCoinsStats* maybe_stats,
115     const CBlockIndex* tip,
116-    AutoFile& afile,
117+    AutoFile&& afile,
118     const fs::path& path,
119     const fs::path& temppath,
120     const std::function<void()>& interruption_point)
121@@ -3244,12 +3251,19 @@ UniValue WriteUTXOSnapshot(
122 UniValue CreateUTXOSnapshot(
123     node::NodeContext& node,
124     Chainstate& chainstate,
125-    AutoFile& afile,
126+    AutoFile&& afile,
127     const fs::path& path,
128     const fs::path& tmppath)
129 {
130     auto [cursor, stats, tip]{WITH_LOCK(::cs_main, return PrepareUTXOSnapshot(chainstate, node.rpc_interruption_point))};
131-    return WriteUTXOSnapshot(chainstate, cursor.get(), &stats, tip, afile, path, tmppath, node.rpc_interruption_point);
132+    return WriteUTXOSnapshot(chainstate,
133+                             cursor.get(),
134+                             &stats,
135+                             tip,
136+                             std::move(afile),
137+                             path,
138+                             tmppath,
139+                             node.rpc_interruption_point);
140 }
141 
142 static RPCHelpMan loadtxoutset()
143diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h
144index b42fe96fd3..0e42bed947 100644
145--- a/src/rpc/blockchain.h
146+++ b/src/rpc/blockchain.h
147@@ -51,7 +51,7 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],
148 UniValue CreateUTXOSnapshot(
149     node::NodeContext& node,
150     Chainstate& chainstate,
151-    AutoFile& afile,
152+    AutoFile&& afile,
153     const fs::path& path,
154     const fs::path& tmppath);
155 
156diff --git a/src/streams.cpp b/src/streams.cpp
157index 3c46530990..2c873e50d3 100644
158--- a/src/streams.cpp
159+++ b/src/streams.cpp
160@@ -119,6 +119,7 @@ bool AutoFile::Commit()
161 
162 bool AutoFile::Truncate(unsigned size)
163 {
164+    m_was_written = true;
165     return ::TruncateFile(m_file, size);
166 }
167 
168diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
169index eebf504d98..d5100d15b1 100644
170--- a/src/test/util/chainstate.h
171+++ b/src/test/util/chainstate.h
172@@ -47,8 +47,11 @@ CreateAndActivateUTXOSnapshot(
173     FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
174     AutoFile auto_outfile{outfile};
175 
176-    UniValue result = CreateUTXOSnapshot(
177-        node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); // Will close auto_outfile.
178+    UniValue result = CreateUTXOSnapshot(node,
179+                                         node.chainman->ActiveChainstate(),
180+                                         std::move(auto_outfile), // Will close auto_outfile.
181+                                         snapshot_path,
182+                                         snapshot_path);
183     LogPrintf(
184         "Wrote UTXO snapshot to %s: %s\n", fs::PathToString(snapshot_path.make_preferred()), result.write());
185

    I have checked remaining explicit closes, couldn’t find any other one that was an AutoFile, but was wondering if we should maybe check these as well:

    I have also rebased the PR locally and ran all tests.

  176. rpc: take ownership of the file by WriteUTXOSnapshot() 
    Have `WriteUTXOSnapshot()` take rvalue reference to make it obvious that
it takes ownership of the file.
    a69c4098b2
  177. Explicitly close all AutoFiles that have been written 
    There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).

So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
    8bb34f07df
  178. util: check that a file has been closed before ~AutoFile() is called 
    If an `AutoFile` has been written to, then expect callers to have closed
it explicitly via the `AutoFile::fclose()` method. This is because if
the destructor calls `std::fclose()` and encounters an error, then it
is too late to indicate this to the caller in a meaningful way.
    4bb5dd78ea
  179. flatfile: check whether the file has been closed successfully c10e382d2a
  180. vasild force-pushed on Jun 16, 2025
  181. vasild commented at 1:34 pm on June 16, 2025: contributor
    f8a7580a08...c10e382d2a: address suggestions
  182. hodlinator approved
  183. hodlinator commented at 1:41 pm on June 16, 2025: contributor

    re-ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702

    Only minor changes in response to feedback since first ACK (https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2924706520)

  184. DrahtBot requested review from l0rinc on Jun 16, 2025
  185. l0rinc commented at 1:49 pm on June 16, 2025: contributor
    ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702 @vasild, what do you think about the remaining fclose calls I mentioned above?
  186. vasild commented at 9:32 am on June 17, 2025: contributor

    what do you think about the remaining fclose calls I mentioned above?

    Here is my take of each one:

    https://github.com/bitcoin/bitcoin/blob/24e5fd3bedcebacbc10f0449be61be636b77dd79/src/util/fs_helpers.cpp#L54

    In this case the file is opened or created and closed immediately, without writing to it. So this cannot cause a data corruption. Still, would be sensible to check fclose() for error and return LockResult::ErrorWrite if it fails.

    https://github.com/bitcoin/bitcoin/blob/00e9b97f37e0bdf4c647236838c10b68b7ad5be3/src/util/readwritefile.cpp#L28 and https://github.com/bitcoin/bitcoin/blob/00e9b97f37e0bdf4c647236838c10b68b7ad5be3/src/util/readwritefile.cpp#L33

    The file is opened for reading only. Maybe it would be better to check fclose() errors and treat them as read errors, but as above, an error from fclose() cannot be an indication that the data we wrote to the disk did not make it to the storage.

    https://github.com/bitcoin/bitcoin/blob/24e5fd3bedcebacbc10f0449be61be636b77dd79/src/util/fs_helpers.cpp#L134

    This code is beyond me, I don’t get it. It opens a file, does fsync() without writing anything and closes the file.

    The fsync() function shall request that all data for the open file descriptor named by fildes is to be transferred to the storage device associated with the file described by fildes.

    There is no data associated with that file descriptor, so what is the point!? Now, the same file might be opened at the same time by another piece of code, but that would be a different file descriptor. Indeed DirectoryCommit() is used in such a way that some of the files it operates on will be opened by the DirectoryCommit()’s caller FlatFileSeq::Flush(). So what happens in FlatFileSeq::Flush() is roughly:

    1. open the file under file descriptor 1
    2. fsync() or fdatasync() on file descriptor 1 via FileCommit()
    3. DirectoryCommit(): open the file directory under file descriptor 2, fsync() file descriptor 2, close file descriptor 2
    4. close file descriptor 1

    I am puzzled :confused: :face_with_head_bandage: edit: now it makes sense, DirectoryCommit() syncs the directory itself, not the file, I was blind. So that fclose() better be checked for an error. This was added in https://github.com/bitcoin/bitcoin/pull/14501

  187. in src/addrdb.cpp:85 in c10e382d2a 
    84     }
85-    fileout.fclose();
86+    if (fileout.fclose() != 0) {
87+        const int errno_save{errno};
88+        remove(pathTmp);
89+        LogError("Failed to close file %s after commit: %s", fs::PathToString(pathTmp), SysErrorString(errno_save));
    maflcko commented at 12:14 pm on June 17, 2025:

    not sure about this. Commit is stronger than fclose, so I fail to see how this could ever fail on a real and normal system.

    Forcing devs to write verbose and dead code doesn’t seem ideal. It would be better to just treat Commit as m_was_written=false.

    hodlinator commented at 7:55 am on June 18, 2025:
    Agree, would make sense to set m_was_written=false when we return true from AutoFile::Commit(). Maybe rename m_was_written to m_pending_output. Then this last if-block could be removed.
    l0rinc commented at 8:00 am on June 18, 2025:

    Maybe rename m_was_written to m_pending_output

    Or just m_dirty

    vasild commented at 12:11 pm on June 23, 2025:

    Are you suggesting to ignore the return value of fileout.fclose() like in master and assume it must have succeeded? Note that the errors of fclose() are a superset of the fflush() errors. In other words, fclose() may fail for errors other than fflush() errors. From fclose man page:

       The fclose() function may also fail and set errno for any of the errors specified for  the  routines  close(2), write(2), or fflush(3).
    maflcko commented at 1:43 pm on June 23, 2025:
    My understanding is that once the file is synced, subsequent IO errors shouldn’t matter, because the file was already synced.
    maflcko commented at 2:38 pm on June 23, 2025:
    If AutoFile::Commit was erroneous, the correct fix would be to fix Commit, not add code to the caller to workaround the error.
    vasild commented at 9:36 am on June 26, 2025:
    I prefer to be on the safe side and handle fclose() errors, it is just a few extra lines.
    maflcko commented at 4:16 pm on June 26, 2025:
    If a Commit is not possible without closing the file, then Commit should close the file. Requiring all call sites of Commit to manually close the file seems verbose and less safe to me.
    hodlinator commented at 6:24 pm on June 26, 2025:

    If Commit() fails and (while failing) were to close the file internally as suggested, it should probably throw an exception after that so calling code doesn’t try to continue using a closed AutoFile.

    An alternative would be to have a AutoFile::CommitAndClose()-function that always closes regardless of errors and clearly signals that calling code does not expect to continue using the AutoFile?

    maflcko commented at 10:40 am on July 4, 2025:

    An alternative would be to have a AutoFile::CommitAndClose()-function that always closes regardless of errors and clearly signals that calling code does not expect to continue using the AutoFile?

    yes, that is what i was trying to suggest

    vasild commented at 5:32 am on July 5, 2025:
    I think the current Commit() is good as it is. It does what it promises - flushes the file data (and checks that the flush succeeded). However a subsequent close is not guaranteed to succeed if commit succeeded before that because close does more than flush.
  188. in src/flatfile.cpp:103 in c10e382d2a 
    100     }
101     if (!FileCommit(file)) {
102-        fclose(file);
103         LogError("%s: failed to commit file %d\n", __func__, pos.nFile);
104+        if (fclose(file) != 0) {
105+            LogError("Failed to close file %d", pos.nFile);
    maflcko commented at 12:19 pm on June 17, 2025:
    same, etc …
  189. DrahtBot requested review from maflcko on Jun 17, 2025
  190. hodlinator commented at 7:56 am on June 18, 2025: contributor

    I am puzzled 😕 🤕 edit: now it makes sense, DirectoryCommit() syncs the directory itself, not the file, I was blind. So that fclose() better be checked for an error. This was added in #14501

    Might need to consider networked FS directory behavior when adding errors (https://github.com/bitcoin/bitcoin/pull/14501#discussion_r546662763)?

  191. luke-jr referenced this in commit a480da233e on Jul 1, 2025
  192. luke-jr referenced this in commit 80a1947178 on Jul 1, 2025
  193. luke-jr referenced this in commit 7b2a513f0e on Jul 1, 2025
  194. luke-jr referenced this in commit 9270eb7ca5 on Jul 1, 2025
  195. achow101 commented at 10:27 pm on July 3, 2025: member
    ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
  196. achow101 merged this on Jul 3, 2025
  197. achow101 closed this on Jul 3, 2025
  198. in src/flatfile.cpp:50 in c10e382d2a 
    45@@ -46,7 +46,9 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only) const
46     }
47     if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {
48         LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path));
49-        fclose(file);
50+        if (fclose(file) != 0) {
51+            LogError("Unable to close file %s", fs::PathToString(path));
    maflcko commented at 10:41 am on July 4, 2025:
    no write has happened, and an error is already logged. Seems redundant to log even more errors?
  199. in src/flatfile.cpp:96 in c10e382d2a 
    90@@ -86,17 +91,24 @@ bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) const
91         return false;
92     }
93     if (finalize && !TruncateFile(file, pos.nPos)) {
94-        fclose(file);
95         LogError("%s: failed to truncate file %d\n", __func__, pos.nFile);
96+        if (fclose(file) != 0) {
97+            LogError("Failed to close file %d", pos.nFile);
    maflcko commented at 10:46 am on July 4, 2025:
    same here. if truncation failed and is logged as error, i don’t think it matters to log another error whether or not closing has failed as well.
  200. fanquake commented at 8:50 am on July 8, 2025: member
    @vasild Are you planning on responding to the (remaining) outstanding comments here? Or opening a followup?
  201. vasild commented at 5:07 am on July 16, 2025: contributor
    I prefer to always check whether a syscall failed even if it seems that it should always succeed. In this particular case, the “seems” is not very strong because close is doing more things than flush or truncate and those things could fail. So I think the current code is solid. I do not see a value in removing a check whether close failed.
  202. vasild deleted the branch on Jul 18, 2025


vasild DrahtBot maflcko achow101 Sjors ryanofsky hodlinator l0rinc fanquake


l0rinc ryanofsky

Labels
Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-10-31 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me