util: explicitly close all AutoFiles that have been written

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:

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.
Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
(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.

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
Concept ACK	ryanofsky, hodlinator
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:

#31551 (optimization: bulk reads(27%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD by l0rinc)
#31539 (optimization: buffer reads(23%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD by l0rinc)
#31519 (refactor: Use std::span over Span by maflcko)
#31490 (refactor: inline UndoWriteToDisk and WriteBlockToDisk to reduce serialization calls by l0rinc)
#31144 (optimization: batch XOR operations 12% faster IBD by l0rinc)
#30987 (Don’t zero-after-free DataStream: Faster IBD on some configurations by davidgumberg)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#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.

DrahtBot added the label Utils/log/libs on Jan 24, 2024

vasild force-pushed on Jan 24, 2024

DrahtBot added the label CI failed on Jan 24, 2024

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

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.

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().

vasild force-pushed on Jan 25, 2024

DrahtBot removed the label CI failed on Jan 25, 2024

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?

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.

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.

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?

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.

vasild commented at 4:40 pm on February 5, 2024: contributor

How/when would the file be closed?

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.

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.

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

DrahtBot added the label CI failed on Mar 12, 2024

Retropex referenced this in commit f54c172ffe on Mar 28, 2024

Retropex referenced this in commit 70403c5bb7 on Mar 28, 2024

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.

DrahtBot marked this as a draft on Apr 23, 2024

DrahtBot commented at 6:50 am on April 23, 2024: contributor

Converted to draft due to failing CI and inactivity

vasild force-pushed on May 10, 2024

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.

vasild marked this as ready for review on May 10, 2024

vasild force-pushed on May 10, 2024

Sjors commented at 4:59 pm on May 10, 2024: member

@vasild CI seems unhappy.

vasild force-pushed on May 13, 2024

vasild marked this as a draft on May 13, 2024

vasild force-pushed on May 13, 2024

DrahtBot removed the label CI failed on May 13, 2024

vasild marked this as ready for review on May 14, 2024

vasild commented at 9:51 am on May 14, 2024: contributor

Fixed the CI!

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:

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

I dropped this change.

vasild force-pushed on May 14, 2024

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().

DrahtBot added the label Needs rebase on May 23, 2024

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.

luke-jr referenced this in commit ec8ef5e51c on Jun 13, 2024

luke-jr referenced this in commit 17f1f24993 on Jun 13, 2024

vasild force-pushed on Jun 20, 2024

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

DrahtBot removed the label Needs rebase on Jun 20, 2024

maflcko commented at 12:04 pm on June 20, 2024: member

lgtm ACK 11be9f4103ea219f801a1f0fe1385f66ca70ad22

Will do some testing later

luke-jr referenced this in commit d2ea6ab60b on Jun 22, 2024

DrahtBot added the label Needs rebase on Jul 2, 2024

vasild force-pushed on Jul 2, 2024

vasild commented at 12:41 pm on July 2, 2024: contributor

11be9f4103...ea89a6e368: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jul 2, 2024

DrahtBot added the label Needs rebase on Jul 9, 2024

vasild force-pushed on Jul 22, 2024

vasild commented at 10:33 am on July 22, 2024: contributor

ea89a6e368...4533445fd7: rebase due to conflicts

DrahtBot added the label CI failed on Jul 22, 2024

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.

DrahtBot removed the label Needs rebase on Jul 22, 2024

vasild commented at 12:56 pm on July 22, 2024: contributor

CI failure seems unrelated.

DrahtBot removed the label CI failed on Jul 22, 2024

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:

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.
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.

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     }

DrahtBot added the label CI failed on Aug 7, 2024

DrahtBot added the label Needs rebase on Aug 21, 2024

vasild force-pushed on Aug 22, 2024

vasild commented at 9:27 am on August 22, 2024: contributor

4533445fd7...1dcd626fe1: rebase due to conflicts

DrahtBot removed the label Needs rebase on Aug 22, 2024

vasild force-pushed on Aug 23, 2024

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

DrahtBot removed the label CI failed on Aug 23, 2024

vasild force-pushed on Sep 2, 2024

vasild commented at 2:54 pm on September 2, 2024: contributor

587d996f07...489c2477e7: rebase to pick CMake

DrahtBot added the label CI failed on Sep 5, 2024

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).

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("%s: Failed to close filter file %d: %s\n", __func__, 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).

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.

DrahtBot removed the label CI failed on Sep 15, 2024

DrahtBot added the label Needs rebase on Sep 17, 2024

util: 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.

dba7835386

vasild force-pushed on Sep 28, 2024

vasild commented at 5:20 am on September 28, 2024: contributor

489c2477e7...dba7835386: rebase due to conflicts

DrahtBot removed the label Needs rebase on Sep 28, 2024

in src/policy/fees.cpp:954 in dba7835386

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));

in src/rpc/blockchain.cpp:53 in dba7835386

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

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)).

DrahtBot added the label Needs rebase on Jan 22, 2025

DrahtBot commented at 6:25 pm on January 22, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Code Coverage & Benchmarks

Reviews

Conflicts

Alternative implementation