BufferedFile: fclose at destruction #29614

luke-jr commented at 6:04 PM on March 10, 2024: member

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 commented at 6:04 PM on March 10, 2024: contributor

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.

in src/streams.h:523 in 0397ee1288 outdated

 519 | @@ -520,6 +520,10 @@ class BufferedFile
 520 |              throw std::ios_base::failure("Rewind limit must be less than buffer size");
 521 |      }
 522 |  
 523 | +    ~BufferedFile() { fclose(); }

vasild commented at 11:28 AM on March 11, 2024:

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 commented at 2:51 PM on March 13, 2024:

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

vasild commented at 6:06 AM on March 28, 2024:

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 commented at 7:35 AM on March 28, 2024:

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

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

and remove this destructor?

vasild commented at 1:42 PM on May 24, 2024:

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

maflcko commented at 12:16 PM on March 11, 2024: member

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 commented at 12:39 PM on March 11, 2024: contributor

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

https://github.com/bitcoin/bitcoin/blob/4a903741b0bc128745b1096586329456d1f1c447/src/test/fuzz/buffered_file.cpp#L22-L23

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 commented at 4:40 PM on March 11, 2024: member

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 force-pushed on Mar 13, 2024

luke-jr commented at 3:06 PM on March 13, 2024: member

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.

luke-jr referenced this in commit 9327929c3a on Mar 13, 2024

DrahtBot added the label CI failed on Mar 13, 2024

luke-jr force-pushed on Mar 14, 2024

DrahtBot removed the label CI failed on Mar 14, 2024

luke-jr referenced this in commit 6fc3f886e2 on Mar 14, 2024

maflcko commented at 6:25 AM on March 28, 2024: member

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

BufferedFile: fclose at destruction

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

88fe778d9d

luke-jr force-pushed on May 15, 2024

DrahtBot added the label CI failed on Sep 11, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

achow101 commented at 2:50 PM on October 15, 2024: member

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.

achow101 closed this on Oct 15, 2024

bitcoin locked this on Oct 15, 2025