On Thu, Nov 13, 2025 at 11:17 AM Bryan Green <[email protected]> wrote: > Corrected master patch and back patch for v16-v18.
Thanks. I wondered what system-generated new handles might appear in a child process and potentially collide with a non-inherited handle's numerical value (perhaps a thread handle or something like that?), but I guess it'd also have to accept a write to confuse the test, which seems unlikely, so that's probably OK. I also hope that the new test could eventually be merged with a general port layer test suite as mentioned earlier. What do you think about these improvements? See attached. * moved and adjusted new comment about flag conversion to cover all three flags, since it's true for all of them * adjusted the comment about why we don't use SetHandleInformation() to highlight that it would be (slightly) leaky * removed O_DIRECT's extra definition from port.h, since it's now in win32_port.h One question is why on earth the open() redirection is in port.h while the "supplements to fcntl.h" are in win32_port.h. Obviously those are tightly coupled. As far as I know there are two forces keeping some Windows porting magic in port.h that we'd ideally isolate in win32_port.h, and this case doesn't appear to qualify for either as far as I can guess, anyway: * some sleep/retry wrappers were thought to be needed by Cygwin too: API-wise it's a POSIX, but it couldn't hide the underlying NT file locking semantics * sometimes we need a later definition time: I recall battling that for #define ftruncate and/or lseek (if you define them before certain system headers are included, you break them) Cygwin's <fcntl.h> defines these flags, if I've found the right file[1], and has its own open() that we're using directly. If it didn't, our code would have failed to compile when Cygwin was being tested in the build farm up until a year or so ago (and it will be tested again soon[2]). So we could probably move at least open() into win32_port.h, in a separate commit. I think it's also quite likely that Cygwin turns on the Windows 10 POSIX directory entry semantics, so even rename() etc could probably be moved over too. (Whether our own porting layer should turn that stuff on too and delete the retry stuff entirely is an open question which no Windows expert has ever opined on, only us Unix hackers battling against random failures in the build farm.) We should probably also set up a CI task for Cygwin if we're keeping support. [1] https://github.com/cygwin/cygwin/blob/main/newlib/libc/include/sys/_default_fcntl.h [2] https://www.postgresql.org/message-id/flat/916d0fd1-a99b-41c4-a017-ff2428bf8cca%40dunslane.net
From a316123d7136223e24c8ab39c92038aafc09ea49 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sun, 30 Nov 2025 11:23:39 +1300 Subject: [PATCH v5] Fix O_CLOEXEC flag handling in Windows port. PostgreSQL's src/port/open.c has always set bInheritHandle = TRUE when opening files on Windows, making all file descriptors inheritable by child processes. This meant the O_CLOEXEC flag, added to many call sites by commit 1da569ca1f (v16), was silently ignored. The original commit included a comment suggesting that our open() replacement doesn't create inheritable handles, but it was a mis- understanding of the code path. In practice, the code was creating inheritable handles in all cases. This hasn't caused widespread problems because most child processes (archive_command, COPY PROGRAM, etc.) operate on file paths passed as arguments rather than inherited file descriptors. Even if a child wanted to use an inherited handle, it would need to learn the numeric handle value, which isn't passed through our IPC mechanisms. Nonetheless, the current behavior is wrong. It violates documented O_CLOEXEC semantics, contradicts our own code comments, and makes PostgreSQL behave differently on Windows than on Unix. It also creates potential issues with future code or security auditing tools. To fix, define O_CLOEXEC to _O_NOINHERIT in master, previously used by O_DSYNC. We use different values in the back branches to preserve existing values. In pgwin32_open_handle() we set bInheritHandle according to whether O_CLOEXEC is specified, for the same atomic semantics as POSIX in multi-threaded programs that create processes. Backpatch-through: 16 Author: Bryan Green <[email protected]> Discussion: https://postgr.es/m/e2b16375-7430-4053-bda3-5d2194ff1880%40gmail.com --- src/include/port.h | 1 - src/include/port/win32_port.h | 19 +- src/port/open.c | 14 +- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_cloexec/Makefile | 30 ++ src/test/modules/test_cloexec/meson.build | 26 ++ .../modules/test_cloexec/t/001_cloexec.pl | 60 ++++ src/test/modules/test_cloexec/test_cloexec.c | 262 ++++++++++++++++++ 9 files changed, 400 insertions(+), 14 deletions(-) create mode 100644 src/test/modules/test_cloexec/Makefile create mode 100644 src/test/modules/test_cloexec/meson.build create mode 100644 src/test/modules/test_cloexec/t/001_cloexec.pl create mode 100644 src/test/modules/test_cloexec/test_cloexec.c diff --git a/src/include/port.h b/src/include/port.h index 159c2bcd7e3..672b880d40b 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -359,7 +359,6 @@ extern bool rmtree(const char *path, bool rmtopdir); * open() and fopen() replacements to allow deletion of open files and * passing of other special options. */ -#define O_DIRECT 0x80000000 extern HANDLE pgwin32_open_handle(const char *, int, bool); extern int pgwin32_open(const char *, int,...); extern FILE *pgwin32_fopen(const char *, const char *); diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index f54ccef7db8..8194714976f 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -335,18 +335,15 @@ extern int _pglstat64(const char *name, struct stat *buf); /* * Supplement to <fcntl.h>. - * This is the same value as _O_NOINHERIT in the MS header file. This is - * to ensure that we don't collide with a future definition. It means - * we cannot use _O_NOINHERIT ourselves. - */ -#define O_DSYNC 0x0080 - -/* - * Our open() replacement does not create inheritable handles, so it is safe to - * ignore O_CLOEXEC. (If we were using Windows' own open(), it might be - * necessary to convert this to _O_NOINHERIT.) + * + * We borrow bits from the high end when we have to, to avoid colliding with + * the system-defined values. Our open() replacement in src/port/open.c + * converts these to the equivalent CreateFile() flags, along with the ones + * from fcntl.h. */ -#define O_CLOEXEC 0 +#define O_CLOEXEC _O_NOINHERIT +#define O_DIRECT 0x80000000 +#define O_DSYNC 0x04000000 /* * Supplement to <errno.h>. diff --git a/src/port/open.c b/src/port/open.c index 4a31c5d7b77..ec0fbf29e92 100644 --- a/src/port/open.c +++ b/src/port/open.c @@ -74,13 +74,23 @@ pgwin32_open_handle(const char *fileName, int fileFlags, bool backup_semantics) /* Check that we can handle the request */ assert((fileFlags & ((O_RDONLY | O_WRONLY | O_RDWR) | O_APPEND | (O_RANDOM | O_SEQUENTIAL | O_TEMPORARY) | - _O_SHORT_LIVED | O_DSYNC | O_DIRECT | + _O_SHORT_LIVED | O_DSYNC | O_DIRECT | O_CLOEXEC | (O_CREAT | O_TRUNC | O_EXCL) | (O_TEXT | O_BINARY))) == fileFlags); sa.nLength = sizeof(sa); - sa.bInheritHandle = TRUE; sa.lpSecurityDescriptor = NULL; + /* + * If O_CLOEXEC is specified, create a non-inheritable handle. Otherwise, + * create an inheritable handle (the default Windows behavior). + * + * Note: We could instead use SetHandleInformation() after CreateFile() to + * clear HANDLE_FLAG_INHERIT, but this way avoids rare leaks in + * multi-threaded programs that create processes, just like POSIX + * O_CLOEXEC. + */ + sa.bInheritHandle = !(fileFlags & O_CLOEXEC); + while ((h = CreateFile(fileName, /* cannot use O_RDONLY, as it == 0 */ (fileFlags & O_RDWR) ? (GENERIC_WRITE | GENERIC_READ) : diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index d079b91b1a2..220e46c3ec3 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -19,6 +19,7 @@ SUBDIRS = \ test_binaryheap \ test_bitmapset \ test_bloomfilter \ + test_cloexec \ test_copy_callbacks \ test_custom_rmgrs \ test_ddl_deparse \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index f5114469b92..f8b4b8343ea 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -18,6 +18,7 @@ subdir('test_aio') subdir('test_binaryheap') subdir('test_bitmapset') subdir('test_bloomfilter') +subdir('test_cloexec') subdir('test_copy_callbacks') subdir('test_custom_rmgrs') subdir('test_ddl_deparse') diff --git a/src/test/modules/test_cloexec/Makefile b/src/test/modules/test_cloexec/Makefile new file mode 100644 index 00000000000..70d38575e26 --- /dev/null +++ b/src/test/modules/test_cloexec/Makefile @@ -0,0 +1,30 @@ +# src/test/modules/test_cloexec/Makefile + +# This module is for Windows only +ifeq ($(PORTNAME),win32) + +MODULE_big = test_cloexec +OBJS = \ + $(WIN32RES) \ + test_cloexec.o + +PGFILEDESC = "test_cloexec - test O_CLOEXEC flag handling" + +# Build as a standalone program, not a shared library +PROGRAM = test_cloexec +override CPPFLAGS := -I$(libpq_srcdir) $(CPPFLAGS) + +TAP_TESTS = 1 + +endif + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_cloexec +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_cloexec/meson.build b/src/test/modules/test_cloexec/meson.build new file mode 100644 index 00000000000..63c8658b04e --- /dev/null +++ b/src/test/modules/test_cloexec/meson.build @@ -0,0 +1,26 @@ +# src/test/modules/test_cloexec/meson.build + +# This test is Windows-only +if host_system != 'windows' + subdir_done() +endif + +test_cloexec_sources = files('test_cloexec.c') + +test_cloexec = executable('test_cloexec', + test_cloexec_sources, + dependencies: [frontend_code], + link_with: [pgport[''], pgcommon['']], + kwargs: default_bin_args, +) + +tests += { + 'name': 'test_cloexec', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'tap': { + 'tests': [ + 't/001_cloexec.pl', + ], + }, +} diff --git a/src/test/modules/test_cloexec/t/001_cloexec.pl b/src/test/modules/test_cloexec/t/001_cloexec.pl new file mode 100644 index 00000000000..4c56eb3a4be --- /dev/null +++ b/src/test/modules/test_cloexec/t/001_cloexec.pl @@ -0,0 +1,60 @@ +# Test O_CLOEXEC flag handling on Windows +# +# This test verifies that file handles opened with O_CLOEXEC are not +# inherited by child processes, while handles opened without O_CLOEXEC +# are inherited. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use Test::More; +use IPC::Run qw(run); +use File::Spec; +use Cwd 'abs_path'; + +if (!$PostgreSQL::Test::Utils::windows_os) +{ + plan skip_all => 'test is Windows-specific'; +} + +plan tests => 1; + +my $test_prog; +foreach my $dir (split(/$Config::Config{path_sep}/, $ENV{PATH})) +{ + my $candidate = File::Spec->catfile($dir, 'test_cloexec.exe'); + if (-f $candidate && -x $candidate) + { + $test_prog = $candidate; + last; + } +} + +if (!$test_prog) +{ + $test_prog = './test_cloexec.exe'; +} + +if (!-f $test_prog) +{ + BAIL_OUT("test program not found: $test_prog"); +} + +note("Using test program: $test_prog"); + +my ($stdout, $stderr); +my $result = run [ $test_prog ], '>', \$stdout, '2>', \$stderr; + +note("Test program output:"); +note($stdout) if $stdout; + +if ($stderr) +{ + diag("Test program stderr:"); + diag($stderr); +} + +ok($result && $stdout =~ /SUCCESS.*O_CLOEXEC behavior verified/s, + "O_CLOEXEC prevents handle inheritance"); + +done_testing(); \ No newline at end of file diff --git a/src/test/modules/test_cloexec/test_cloexec.c b/src/test/modules/test_cloexec/test_cloexec.c new file mode 100644 index 00000000000..9f645451684 --- /dev/null +++ b/src/test/modules/test_cloexec/test_cloexec.c @@ -0,0 +1,262 @@ +/*------------------------------------------------------------------------- + * + * test_cloexec.c + * Test O_CLOEXEC flag handling on Windows + * + * This program tests that: + * 1. File handles opened with O_CLOEXEC are NOT inherited by child processes + * 2. File handles opened without O_CLOEXEC ARE inherited by child processes + * + * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group + * + *------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include <fcntl.h> +#include <sys/stat.h> + +#ifdef WIN32 +#include <windows.h> +#endif + +#include "common/file_utils.h" +#include "port.h" + +static void run_parent_tests(const char *testfile1, const char *testfile2); +static void run_child_tests(const char *handle1_str, const char *handle2_str); +static bool try_write_to_handle(HANDLE h, const char *label); + +int +main(int argc, char *argv[]) +{ + char testfile1[MAXPGPATH]; + char testfile2[MAXPGPATH]; + + /* Windows-only test */ +#ifndef WIN32 + fprintf(stderr, "This test only runs on Windows\n"); + return 0; +#endif + + if (argc == 3) + { + /* + * Child mode: receives two handle values as hex strings and attempts + * to write to them. + */ + run_child_tests(argv[1], argv[2]); + return 0; + } + else if (argc == 1) + { + /* Parent mode: opens files and spawns child */ + snprintf(testfile1, sizeof(testfile1), "test_cloexec_1_%d.tmp", (int) getpid()); + snprintf(testfile2, sizeof(testfile2), "test_cloexec_2_%d.tmp", (int) getpid()); + + run_parent_tests(testfile1, testfile2); + + /* Clean up test files */ + unlink(testfile1); + unlink(testfile2); + + return 0; + } + else + { + fprintf(stderr, "Usage: %s [handle1_hex handle2_hex]\n", argv[0]); + return 1; + } +} + +static void +run_parent_tests(const char *testfile1, const char *testfile2) +{ +#ifdef WIN32 + int fd1, + fd2; + HANDLE h1, + h2; + char cmdline[1024]; + STARTUPINFO si; + PROCESS_INFORMATION pi; + DWORD exit_code; + + printf("Parent: Opening test files...\n"); + + /* + * Open first file WITH O_CLOEXEC - should NOT be inherited + */ + fd1 = open(testfile1, O_RDWR | O_CREAT | O_TRUNC | O_CLOEXEC, 0600); + if (fd1 < 0) + { + fprintf(stderr, "Failed to open %s: %s\n", testfile1, strerror(errno)); + exit(1); + } + + /* + * Open second file WITHOUT O_CLOEXEC - should be inherited + */ + fd2 = open(testfile2, O_RDWR | O_CREAT | O_TRUNC, 0600); + if (fd2 < 0) + { + fprintf(stderr, "Failed to open %s: %s\n", testfile2, strerror(errno)); + close(fd1); + exit(1); + } + + /* Get Windows HANDLEs from file descriptors */ + h1 = (HANDLE) _get_osfhandle(fd1); + h2 = (HANDLE) _get_osfhandle(fd2); + + if (h1 == INVALID_HANDLE_VALUE || h2 == INVALID_HANDLE_VALUE) + { + fprintf(stderr, "Failed to get OS handles\n"); + close(fd1); + close(fd2); + exit(1); + } + + printf("Parent: fd1=%d (O_CLOEXEC) -> HANDLE=%p\n", fd1, h1); + printf("Parent: fd2=%d (no O_CLOEXEC) -> HANDLE=%p\n", fd2, h2); + + /* + * Spawn child process with bInheritHandles=TRUE, passing handle values as + * hex strings + */ + snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p", + GetCommandLine(), h1, h2); + + /* + * Find the actual executable path by removing any arguments from + * GetCommandLine(). + */ + { + char exe_path[MAX_PATH]; + char *space_pos; + + GetModuleFileName(NULL, exe_path, sizeof(exe_path)); + snprintf(cmdline, sizeof(cmdline), "\"%s\" %p %p", + exe_path, h1, h2); + } + + memset(&si, 0, sizeof(si)); + si.cb = sizeof(si); + memset(&pi, 0, sizeof(pi)); + + printf("Parent: Spawning child process...\n"); + printf("Parent: Command line: %s\n", cmdline); + + if (!CreateProcess(NULL, /* application name */ + cmdline, /* command line */ + NULL, /* process security attributes */ + NULL, /* thread security attributes */ + TRUE, /* bInheritHandles - CRITICAL! */ + 0, /* creation flags */ + NULL, /* environment */ + NULL, /* current directory */ + &si, /* startup info */ + &pi)) /* process information */ + { + fprintf(stderr, "CreateProcess failed: %lu\n", GetLastError()); + close(fd1); + close(fd2); + exit(1); + } + + printf("Parent: Waiting for child process...\n"); + + /* Wait for child to complete */ + WaitForSingleObject(pi.hProcess, INFINITE); + GetExitCodeProcess(pi.hProcess, &exit_code); + + CloseHandle(pi.hProcess); + CloseHandle(pi.hThread); + + close(fd1); + close(fd2); + + printf("Parent: Child exit code: %lu\n", exit_code); + + if (exit_code == 0) + printf("Parent: SUCCESS - O_CLOEXEC behavior verified\n"); + else + { + printf("Parent: FAILURE - O_CLOEXEC not working correctly\n"); + exit(1); + } +#endif +} + +static void +run_child_tests(const char *handle1_str, const char *handle2_str) +{ +#ifdef WIN32 + HANDLE h1, + h2; + bool h1_worked, + h2_worked; + + /* Parse handle values from hex strings */ + if (sscanf(handle1_str, "%p", &h1) != 1 || + sscanf(handle2_str, "%p", &h2) != 1) + { + fprintf(stderr, "Child: Failed to parse handle values\n"); + exit(1); + } + + printf("Child: Received HANDLE1=%p (should fail - O_CLOEXEC)\n", h1); + printf("Child: Received HANDLE2=%p (should work - no O_CLOEXEC)\n", h2); + + /* Try to write to both handles */ + h1_worked = try_write_to_handle(h1, "HANDLE1"); + h2_worked = try_write_to_handle(h2, "HANDLE2"); + + printf("Child: HANDLE1 (O_CLOEXEC): %s\n", + h1_worked ? "ACCESSIBLE (BAD!)" : "NOT ACCESSIBLE (GOOD!)"); + printf("Child: HANDLE2 (no O_CLOEXEC): %s\n", + h2_worked ? "ACCESSIBLE (GOOD!)" : "NOT ACCESSIBLE (BAD!)"); + + /* + * For O_CLOEXEC to work correctly, h1 should NOT be accessible (h1_worked + * == false) and h2 SHOULD be accessible (h2_worked == true). + */ + if (!h1_worked && h2_worked) + { + printf("Child: Test PASSED - O_CLOEXEC working correctly\n"); + exit(0); + } + else + { + printf("Child: Test FAILED - O_CLOEXEC not working correctly\n"); + exit(1); + } +#endif +} + +static bool +try_write_to_handle(HANDLE h, const char *label) +{ +#ifdef WIN32 + const char *test_data = "test\n"; + DWORD bytes_written; + BOOL result; + + result = WriteFile(h, test_data, strlen(test_data), &bytes_written, NULL); + + if (result && bytes_written == strlen(test_data)) + { + printf("Child: Successfully wrote to %s\n", label); + return true; + } + else + { + printf("Child: Failed to write to %s (error %lu)\n", + label, GetLastError()); + return false; + } +#else + return false; +#endif +} -- 2.51.2
