log: check fclose() results and report safely in logging.cpp #33646

pull cedwies wants to merge 1 commits into bitcoin:master from cedwies:log-safe-fclose changing 1 files +47 −14

cedwies commented at 7:06 PM on October 17, 2025: contributor
fclose() can report write errors (for example if the disk is full or the filesystem has a problem). Right now, some fclose() calls in src/logging.cpp ignore the return value. This means errors might go unnoticed and log lines could be lost without warning.

What this PR does:
- Add a small helper that prints fclose() errors to stderr (with path and errno).
- In shutdown: close m_fileout safely and report errors.
- In reopen: open the new file, swap it in, close the old one, and report errors if closing fails.
- In shrink/rotate: check all fclose(file) calls and report failures.
No other behavior changes. Normal logging, rotation, and console output remain unchanged.
DrahtBot added the label Utils/log/libs on Oct 17, 2025
DrahtBot commented at 7:06 PM on October 17, 2025: 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/33646.



Reviews

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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #34809 (threadsafety: Add STDLOCK() macro for StdMutex by ajtowns)
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.
cedwies closed this on Oct 17, 2025
cedwies reopened this on Oct 17, 2025
cedwies force-pushed on Oct 17, 2025
cedwies force-pushed on Oct 17, 2025
DrahtBot added the label Needs rebase on Dec 2, 2025
DrahtBot removed the label Needs rebase on Dec 2, 2025
in src/logging.cpp:130 in 29a52aa157 outdated
```
 134 | +        m_cur_buffer_memusage = 0;
 135 | +        m_buffer_lines_discarded = 0;
 136 | +        m_msgs_before_open.clear();
 137 | +    }
 138 | +    if (f && fclose(f) != 0) {
 139 | +        ReportLogIOError("fclose", m_file_path);
```
ajtowns commented at 2:46 AM on December 11, 2025:

What's the value in having this outside the m_cs guard?

cedwies commented at 1:30 PM on December 11, 2025:

I put the fclose outside the lock to avoid holding the mutex during potentially slow I/O (like flushing the file). We swap m_fileout inside, so the state is safe, and then close the old file without blocking other threads. ReportLogIOError is designed to be lock-free anyway, if I am not missing something.
ajtowns commented at 3:09 AM on December 11, 2025: contributor

Should rebase on top of master rather than including a merge commit.

log: check fclose() results and report safely

Some buffered I/O errors surface only on fclose(). Several fclose() calls in
src/logging.cpp ignored the return value which can risk silent loss of log lines
or misleading success during rotate/truncate.

Check return of fclose() and include errno in diagnostics. For the active log sink,
detach under lock and close outside the lock. On failure, report directly
to stderr (which avoids recursive logging) and flush. No behavior change on success.

72e0cbc266

cedwies force-pushed on Dec 11, 2025
sedited commented at 8:46 AM on March 5, 2026: contributor

This looks ok, but is there a way to consistently trigger an fclose error so this might be tested locally?
DrahtBot added the label Needs rebase on Mar 24, 2026
DrahtBot commented at 10:48 AM on March 24, 2026: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
maflcko commented at 11:01 AM on March 24, 2026: member

This looks ok, but is there a way to consistently trigger an fclose error so this might be tested locally?

Obviously, a logic error (like erroneous double-fclose) can be added manually inside the code. An alternative to mock the fs to return an IO error likely won't work, because there is no buffer to be flushed (setbuf(new_fileout, nullptr); // unbuffered), so any IO error will either happen on the write, or never? So I guess this only leaves manual strace fault injection?

Contributors

Labels

Utils/log/libs Needs rebase

log: check fclose() results and report safely in logging.cpp #33646

What this PR does:

Code Coverage & Benchmarks

Reviews

Conflicts