Skip to content

Commit b13b985

Browse files
authored
Fix stderr closure bug in LogFile::check_fd() using fstat() (#12407)
The check_fd() function had a critical bug where it would close and reopen log files to verify existence, which could inadvertently close stderr/stdout when logging to these special files, breaking error reporting for the entire process. Fixed by replacing the unsafe access() + close/reopen pattern with fstat() on the open file descriptor. This approach: - Uses st_nlink == 0 to detect when regular files have been unlinked - Never triggers for special files like stderr/stdout (they maintain st_nlink > 0) - Eliminates string comparisons and special-case handling - Provides universal safety across all file descriptor types - Maintains detection of externally moved/deleted log files for rotation
1 parent 5b0d86c commit b13b985

2 files changed

Lines changed: 30 additions & 15 deletions

File tree

src/proxy/logging/LogFile.cc

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -712,8 +712,8 @@ LogFile::writeln(char *data, int len, int fd, const char *path)
712712
LogFile::check_fd
713713
714714
This routine will occasionally stat the current logfile to make sure that
715-
it really does exist. The easiest way to do this is to close the file
716-
and re-open it, which will create the file if it doesn't already exist.
715+
it really does exist. We use fstat() on the open file descriptor to check
716+
if the file has been unlinked.
717717
718718
Failure to open the logfile will generate a manager alarm and a Warning.
719719
-------------------------------------------------------------------------*/
@@ -726,13 +726,34 @@ LogFile::check_fd()
726726

727727
if ((stat_check_count % Log::config->file_stat_frequency) == 0) {
728728
//
729-
// It's time to see if the file really exists. If we can't see
730-
// the file (via access), then we'll close our descriptor and
731-
// attempt to re-open it, which will create the file if it's not
732-
// there.
729+
// Check if the file still exists using fstat() on the open file descriptor.
730+
// For regular files that have been unlinked (e.g., by log rotation tools),
731+
// st_nlink will be 0. Non-regular files like stderr/stdout will never have
732+
// st_nlink == 0, so this approach works safely for all file types.
733733
//
734-
if (m_name && !LogFile::exists(m_name)) {
735-
close_file();
734+
if (m_name && is_open()) {
735+
int fd = get_fd();
736+
if (fd >= 0) {
737+
struct stat st;
738+
if (fstat(fd, &st) == 0) {
739+
// If the file has been unlinked, st_nlink will be 0
740+
// This only happens for regular files, not special files like stderr/stdout
741+
if (st.st_nlink == 0) {
742+
close_file();
743+
}
744+
} else {
745+
// fstat failed, the file descriptor may be invalid
746+
// Fall back to path-based existence check
747+
if (!LogFile::exists(m_name)) {
748+
close_file();
749+
}
750+
}
751+
} else {
752+
// No valid fd, fall back to path-based check
753+
if (!LogFile::exists(m_name)) {
754+
close_file();
755+
}
756+
}
736757
}
737758
stat_check_count = 0;
738759
}

tests/gold_tests/logging/log-filenames.test.py

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -258,10 +258,4 @@ def __init__(self):
258258
CustomNamedTest()
259259
stdoutTest()
260260

261-
# The following stderr test can be run successfully by hand using the replay
262-
# files from the sandbox. All the expected output goes to stderr. However, for
263-
# some reason during the AuTest run, the stderr output stops emitting after the
264-
# logging.yaml file is parsed. This is left here for now because it is valuable
265-
# for use during development, but it is left commented out so that it doesn't
266-
# produce the false failure in CI and developer test runs.
267-
# stderrTest()
261+
stderrTest()

0 commit comments

Comments
 (0)