BufferedFile: fclose at destruction

[luke-jr]

This is currently indirectly implied by src/bench/load_external.cpp:LoadExternalBlockFile
The file will be closed by LoadExternalBlockFile().

It reveals a subtle (but noop currently) bug in the BufferedFile fuzz tests: Because the BufferedFile is created before the CAutoFile, the CAutoFile gets cleaned up first, leaving the BufferedFile with pointers to a deleted CAutoFile. The simple fix for this is to use the newly-added fclose for BufferedFile, and both are trivial changes, so I've squashed them into a single commit.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

[vasild]

It is better to check the return value of fclose(), otherwise an IO error may remain unnoticed and unreported, leading to silent data corruption.

The problem with calling fclose() from the destructor is that there is no way to signal a failure to the caller - the destructor does not return a value, does not take arguments and is not supposed to throw either.

fclose() from a destructor is a design issue, better avoid that. #29307 is related.

[luke-jr]

BufferedFile is only used for reading, so I don't think fclose can fail in a meaningful way? #29307 also ignores the result...

[vasild]

BufferedFile is only used for reading

Is it possible to enforce that in some way? Also that the parent AutoFile has been used only for reading prior to passing it to BufferedFile?

#29307 also ignores the result

#29307 fixes/improves some things, but not that

[vasild]

I see that now you explicitly close the file. Maybe assert there like:

assert(opt_buffered_file->fclose() == 0);

and remove this destructor?

[vasild]

Is it not the case that now this destructor is not needed because you explicitly call fclose()?

[maflcko]

This is currently indirectly implied by src/bench/load_external.cpp:LoadExternalBlockFile
The file will be closed by LoadExternalBlockFile().

This comment is wrong, no? If not, it would be good to point to the exact line in the source code that closes the file in LoadExternalBlockFile.

If you are trying to say that real code should be changed to accommodate a comment in a test, I am not sure, unless there is also a real reason for the change.

[vasild]

I agree that there is a subtle error in FUZZ_TARGET(buffered_file):

bitcoin/src/test/fuzz/buffered_file.cpp

Lines 22 to 23 in 4a90374

	std::optional<BufferedFile> opt_buffered_file;
	AutoFile fuzzed_file{

because at the end of the function fuzzed_file will be destroyed and then opt_buffered_file will reference a deleted object. "Luckily" opt_buffered_file is destroyed immediately afterwards without any operations being performed on it.

I do not get the comment about bench/load_external.cpp

[maflcko]

In any case, you are adding the bug here, not fixing it. See the CI, which is failing

    [4256, 4272) 'fuzzed_data_provider' (line 20)
    [4288, 4304) 'fuzzed_file_provider' (line 21)
    [4320, 4392) 'opt_buffered_file' (line 22)
    [4432, 4464) 'fuzzed_file' (line 23) <== Memory access at offset 4432 is inside this variable
    [4496, 4520) 'agg.tmp'
    [4560, 4576) 'ref.tmp' (line 23)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope src/./streams.h:417:24 in AutoFile::release()
Shadow bytes around the buggy address:
  0x7fc119340e80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x7fc119340f00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x7fc119340f80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x7fc119341000: f8 f8 f8 f8 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2
  0x7fc119341080: f2 f2 f2 f2 00 00 f2 f2 00 00 f2 f2 00 00 00 00
=>0x7fc119341100: 00 00 00 00 00 f2 f2 f2 f2 f2[f8]f8 f8 f8 f2 f2
  0x7fc119341180: f2 f2 00 00 00 f2 f2 f2 f2 f2 f8 f8 f3 f3 f3 f3
  0x7fc119341200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fc119341280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fc119341300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7fc119341380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==18245==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x5c,0x25,0x94,0xff,0x7e,
\\%\224\377~

[luke-jr]

If you are trying to say that real code should be changed to accommodate a comment in a test, I am not sure, unless there is also a real reason for the change.

It seems like logically correct behaviour to expect.

In any case, you are adding the bug here, not fixing it. See the CI, which is failing

Yes, since the destructor calls fclose unconditionally, the AutoFile is still gone even though we fclose'd manually already. Fixed by resetting the std::optional too.

[maflcko]

For context, BufferedFile may be better off to be removed wholesale, see also #28226 (comment)

[achow101]

This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs.

Closing due to lack of interest.