From e4157be508d2e92b7e91105df4eeb72c845f0f40 Mon Sep 17 00:00:00 2001 From: jake champion Date: Thu, 31 Jul 2025 09:20:57 +0100 Subject: [PATCH 1/2] Fix stderr closure bug in LogFile::check_fd() using fstat() 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 --- src/proxy/logging/LogFile.cc | 37 ++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/proxy/logging/LogFile.cc b/src/proxy/logging/LogFile.cc index 86addcb66bb..f7a7004c60d 100644 --- a/src/proxy/logging/LogFile.cc +++ b/src/proxy/logging/LogFile.cc @@ -712,8 +712,8 @@ LogFile::writeln(char *data, int len, int fd, const char *path) LogFile::check_fd This routine will occasionally stat the current logfile to make sure that - it really does exist. The easiest way to do this is to close the file - and re-open it, which will create the file if it doesn't already exist. + it really does exist. We use fstat() on the open file descriptor to check + if the file has been unlinked. Failure to open the logfile will generate a manager alarm and a Warning. -------------------------------------------------------------------------*/ @@ -726,13 +726,34 @@ LogFile::check_fd() if ((stat_check_count % Log::config->file_stat_frequency) == 0) { // - // It's time to see if the file really exists. If we can't see - // the file (via access), then we'll close our descriptor and - // attempt to re-open it, which will create the file if it's not - // there. + // Check if the file still exists using fstat() on the open file descriptor. + // For regular files that have been unlinked (e.g., by log rotation tools), + // st_nlink will be 0. Non-regular files like stderr/stdout will never have + // st_nlink == 0, so this approach works safely for all file types. // - if (m_name && !LogFile::exists(m_name)) { - close_file(); + if (m_name && is_open()) { + int fd = get_fd(); + if (fd >= 0) { + struct stat st; + if (fstat(fd, &st) == 0) { + // If the file has been unlinked, st_nlink will be 0 + // This only happens for regular files, not special files like stderr/stdout + if (st.st_nlink == 0) { + close_file(); + } + } else { + // fstat failed, the file descriptor may be invalid + // Fall back to path-based existence check + if (!LogFile::exists(m_name)) { + close_file(); + } + } + } else { + // No valid fd, fall back to path-based check + if (!LogFile::exists(m_name)) { + close_file(); + } + } } stat_check_count = 0; } From 912fae000697b537eb294ecba3680e2d8067846b Mon Sep 17 00:00:00 2001 From: jake champion Date: Wed, 3 Sep 2025 10:19:48 +0100 Subject: [PATCH 2/2] Enables stderr test in logging test suite --- tests/gold_tests/logging/log-filenames.test.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/gold_tests/logging/log-filenames.test.py b/tests/gold_tests/logging/log-filenames.test.py index a4fdbc73b38..5ec6d6d685d 100644 --- a/tests/gold_tests/logging/log-filenames.test.py +++ b/tests/gold_tests/logging/log-filenames.test.py @@ -258,10 +258,4 @@ def __init__(self): CustomNamedTest() stdoutTest() -# The following stderr test can be run successfully by hand using the replay -# files from the sandbox. All the expected output goes to stderr. However, for -# some reason during the AuTest run, the stderr output stops emitting after the -# logging.yaml file is parsed. This is left here for now because it is valuable -# for use during development, but it is left commented out so that it doesn't -# produce the false failure in CI and developer test runs. -# stderrTest() +stderrTest()