From d21895df67eea1d624e1ffc6eaa7a2c86ef386ff Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 25 Mar 2026 10:46:44 -0400 Subject: [PATCH v8 1/2] Fix file descriptor leakages in pg_waldump. TarWALDumpCloseSegment was of the opinion that it didn't need to do anything. It was mistaken: it has to close the open file if any, because nothing else will. In addition, we failed to ensure that any file being read by the XLogReader machinery gets closed before the atexit callback tries to cleanup the temporary directory holding spilled WAL files. While the file would have been closed already in case of a success exit, this doesn't happen in case of pg_fatal() exits. The least messy way to fix that is to move the atexit function into pg_waldump.c, where it has easier access to the XLogReaderState pointer and to WALDumpCloseSegment. These FD leakages are pretty insignificant on Unix-ish platforms, but they're a bug on Windows, because they prevent successful cleanup of the temporary directory for extracted WAL files. (Windows can't delete a directory that holds a deleted-but-still-open file.) This is visible in occasional buildfarm failures. --- src/bin/pg_waldump/archive_waldump.c | 26 ++-------------- src/bin/pg_waldump/pg_waldump.c | 44 ++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c index 96f4d94449f..b8e9754b729 100644 --- a/src/bin/pg_waldump/archive_waldump.c +++ b/src/bin/pg_waldump/archive_waldump.c @@ -91,7 +91,6 @@ static ArchivedWALFile *get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo); static bool read_archive_file(XLogDumpPrivate *privateInfo); static void setup_tmpwal_dir(const char *waldir); -static void cleanup_tmpwal_dir_atexit(void); static FILE *prepare_tmp_write(const char *fname, XLogDumpPrivate *privateInfo); static void perform_tmp_write(const char *fname, StringInfo buf, FILE *file); @@ -607,23 +606,10 @@ setup_tmpwal_dir(const char *waldir) pg_log_debug("created directory \"%s\"", TmpWalSegDir); } -/* - * Remove temporary directory at exit, if any. - */ -static void -cleanup_tmpwal_dir_atexit(void) -{ - Assert(TmpWalSegDir != NULL); - - rmtree(TmpWalSegDir, true); - - TmpWalSegDir = NULL; -} - /* * Open a file in the temporary spill directory for writing an out-of-order - * WAL segment, creating the directory and registering the cleanup callback - * if not already done. Returns the open file handle. + * WAL segment, creating the directory if not already done. + * Returns the open file handle. */ static FILE * prepare_tmp_write(const char *fname, XLogDumpPrivate *privateInfo) @@ -631,15 +617,9 @@ prepare_tmp_write(const char *fname, XLogDumpPrivate *privateInfo) char fpath[MAXPGPATH]; FILE *file; - /* - * Setup temporary directory to store WAL segments and set up an exit - * callback to remove it upon completion if not already. - */ + /* Setup temporary directory to store WAL segments, if we didn't already */ if (unlikely(TmpWalSegDir == NULL)) - { setup_tmpwal_dir(privateInfo->archive_dir); - atexit(cleanup_tmpwal_dir_atexit); - } snprintf(fpath, MAXPGPATH, "%s/%s", TmpWalSegDir, fname); diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 630d9859882..6bda3c12aa3 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -42,6 +42,8 @@ static const char *progname; static volatile sig_atomic_t time_to_stop = false; +static XLogReaderState *xlogreader_state_cleanup = NULL; + static const RelFileLocator emptyRelFileLocator = {0, 0, 0}; typedef struct XLogDumpConfig @@ -454,13 +456,14 @@ TarWALDumpOpenSegment(XLogReaderState *state, XLogSegNo nextSegNo, /* * pg_waldump's XLogReaderRoutine->segment_close callback to support dumping - * WAL files from tar archives. Segment tracking is handled by - * TarWALDumpReadPage, so no action is needed here. + * WAL files from tar archives. Same as wal_segment_close. */ static void TarWALDumpCloseSegment(XLogReaderState *state) { - /* No action needed */ + close(state->seg.ws_file); + /* need to check errno? */ + state->seg.ws_file = -1; } /* @@ -865,6 +868,27 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogStats *stats) total_len, "[100%]"); } +/* + * Remove temporary directory at exit, if any. + */ +static void +cleanup_tmpwal_dir_atexit(void) +{ + /* + * Before calling rmtree, we must close any open file we have in the temp + * directory; else rmdir fails on Windows. + */ + if (xlogreader_state_cleanup != NULL && + xlogreader_state_cleanup->seg.ws_file >= 0) + WALDumpCloseSegment(xlogreader_state_cleanup); + + if (TmpWalSegDir != NULL) + { + rmtree(TmpWalSegDir, true); + TmpWalSegDir = NULL; + } +} + static void usage(void) { @@ -1383,6 +1407,14 @@ main(int argc, char **argv) if (!xlogreader_state) pg_fatal("out of memory while allocating a WAL reading processor"); + /* + * Set up atexit cleanup of temporary directory. This must happen before + * archive_waldump.c could possibly create the temporary directory. Also + * arm the callback to cleanup the xlogreader state. + */ + atexit(cleanup_tmpwal_dir_atexit); + xlogreader_state_cleanup = xlogreader_state; + /* first find a valid recptr to start from */ first_record = XLogFindNextRecord(xlogreader_state, private.startptr, &errormsg); @@ -1495,6 +1527,12 @@ main(int argc, char **argv) LSN_FORMAT_ARGS(xlogreader_state->ReadRecPtr), errormsg); + /* + * Disarm atexit cleanup of open WAL file; XLogReaderFree will close it, + * and we don't want the atexit callback trying to touch freed memory. + */ + xlogreader_state_cleanup = NULL; + XLogReaderFree(xlogreader_state); if (private.archive_name) -- 2.43.7