From bea32ccc8275d5df8f6b7eb5e4b7b37124078873 Mon Sep 17 00:00:00 2001 From: Bryan Green Date: Fri, 31 Oct 2025 10:11:30 -0600 Subject: [PATCH v2] 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, was silently ignored. The original commit included a comment suggesting that our open() replacement doesn't create inheritable handles, but it appears this may have been based on a misunderstanding 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 a nonzero value (0x80000, in the private use range for open() flags). Then honor it in pgwin32_open_handle() by setting bInheritHandle according to whether O_CLOEXEC is specified. We set bInheritHandle directly in SECURITY_ATTRIBUTES rather than using SetHandleInformation() afterwards, as the former is simpler. Author: Bryan Green --- src/include/port/win32_port.h | 17 +- src/port/open.c | 13 +- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_cloexec/Makefile | 30 ++ src/test/modules/test_cloexec/README | 105 +++++++ src/test/modules/test_cloexec/meson.build | 27 ++ .../modules/test_cloexec/t/001_cloexec.pl | 60 ++++ src/test/modules/test_cloexec/test_cloexec.c | 267 ++++++++++++++++++ 9 files changed, 510 insertions(+), 11 deletions(-) create mode 100644 src/test/modules/test_cloexec/Makefile create mode 100644 src/test/modules/test_cloexec/README 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/win32_port.h b/src/include/port/win32_port.h index ff7028bdc8..ad442e1de7 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -335,19 +335,18 @@ extern int _pglstat64(const char *name, struct stat *buf); /* * Supplement to . - * 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 +#define O_DIRECT 0x80000000 +#define O_DSYNC 0x04000000 /* - * 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.) + * Map O_CLOEXEC to Windows' _O_NOINHERIT flag. Our src/port/open.c + * explicitly checks for this flag and sets bInheritHandle accordingly + * when calling CreateFile(). Note that CreateFile() doesn't understand + * this flag natively - we must handle it in our wrapper. */ -#define O_CLOEXEC 0 - +#define O_CLOEXEC _O_NOINHERIT /* * Supplement to . * diff --git a/src/port/open.c b/src/port/open.c index 4a31c5d7b7..71344b9917 100644 --- a/src/port/open.c +++ b/src/port/open.c @@ -74,13 +74,22 @@ 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 setting bInheritHandle=FALSE is simpler + * and achieves the same result. + */ + sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE; + 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 902a795410..3a8e25fafd 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -18,6 +18,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 14fc761c4c..e9309ecb3e 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -17,6 +17,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 0000000000..af885dd3a8 --- /dev/null +++ b/src/test/modules/test_cloexec/Makefile @@ -0,0 +1,30 @@ +# src/test/modules/test_cloexec/Makefile + +MODULE_big = test_cloexec +OBJS = \ + $(WIN32RES) \ + test_cloexec.o + +PGFILEDESC = "test_cloexec - test O_CLOEXEC flag handling" + +# This module is for Windows only +ifeq ($(PORTNAME),win32) + +# 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/README b/src/test/modules/test_cloexec/README new file mode 100644 index 0000000000..1d518eed02 --- /dev/null +++ b/src/test/modules/test_cloexec/README @@ -0,0 +1,105 @@ +test_cloexec - Test O_CLOEXEC flag handling on Windows +======================================================= + +This test module verifies that the O_CLOEXEC flag works correctly on Windows +after the fix in src/port/open.c. + +Test Overview +------------- + +The test verifies two critical behaviors: + +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 + +How It Works +------------ + +The test consists of a standalone C program (test_cloexec.c) that: + +1. Parent Process: + - Opens two test files: + * File 1: with O_CLOEXEC flag (should NOT be inherited) + * File 2: without O_CLOEXEC flag (should be inherited) + - Gets the Windows HANDLE values for both file descriptors + - Spawns a child process with bInheritHandles=TRUE + - Passes both HANDLE values to the child as command-line arguments + - Waits for the child to complete and checks the exit code + +2. Child Process: + - Receives the two HANDLE values as hex strings + - Attempts to write to both handles using WriteFile() + - Reports which handles were accessible + - Exits with status 0 if O_CLOEXEC worked correctly: + * Handle 1 (O_CLOEXEC) should fail (not inherited) + * Handle 2 (no O_CLOEXEC) should succeed (inherited) + +Test Files +---------- + +- test_cloexec.c - The test program (parent and child logic) +- t/001_cloexec.pl - TAP test script that runs the program +- Makefile - Build rules for make-based builds +- meson.build - Build rules for meson-based builds + +Integration with PostgreSQL +---------------------------- + +This test should be placed in: + src/test/modules/test_cloexec/ + +The test is Windows-only and will be skipped on other platforms. + +To run the test: + make check + or + meson test test_cloexec + +Expected Output +--------------- + +On success, the test program outputs: + Parent: Opening test files... + Parent: fd1=3 (O_CLOEXEC) -> HANDLE=0x... + Parent: fd2=4 (no O_CLOEXEC) -> HANDLE=0x... + Parent: Spawning child process... + Parent: Waiting for child process... + Child: Received HANDLE1=0x... (should fail - O_CLOEXEC) + Child: Received HANDLE2=0x... (should work - no O_CLOEXEC) + Child: Failed to write to HANDLE1 (error 6) + Child: Successfully wrote to HANDLE2 + Child: HANDLE1 (O_CLOEXEC): NOT ACCESSIBLE (GOOD!) + Child: HANDLE2 (no O_CLOEXEC): ACCESSIBLE (GOOD!) + Child: Test PASSED - O_CLOEXEC working correctly + Parent: Child exit code: 0 + Parent: SUCCESS - O_CLOEXEC behavior verified + +Technical Details +----------------- + +The test uses Windows-specific APIs: + +- _get_osfhandle() - Convert CRT file descriptor to Windows HANDLE +- CreateProcess() - Spawn child with bInheritHandles=TRUE +- WriteFile() - Attempt to write to handles in child process + +The test relies on the fact that: +- Windows Error 6 (ERROR_INVALID_HANDLE) indicates the handle is not accessible +- Inherited handles work across process boundaries when properly set up +- O_CLOEXEC should prevent inheritance by setting bInheritHandle=FALSE + +Relation to the Patch +--------------------- + +This test validates the changes made in the O_CLOEXEC patch: + +1. src/include/port/win32_port.h: + - Changed O_CLOEXEC from 0 to _O_NOINHERIT (0x80) + +2. src/port/open.c (pgwin32_open_handle): + - Added O_CLOEXEC to the assertion of handled flags + - Changed bInheritHandle from always TRUE to conditional: + sa.bInheritHandle = (fileFlags & O_CLOEXEC) ? FALSE : TRUE; + +Without these changes, both handles would be inherited by the child, +and the test would fail. diff --git a/src/test/modules/test_cloexec/meson.build b/src/test/modules/test_cloexec/meson.build new file mode 100644 index 0000000000..f6a3087bfe --- /dev/null +++ b/src/test/modules/test_cloexec/meson.build @@ -0,0 +1,27 @@ +# 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, + include_directories: [postgres_inc], + link_with: [pgport[''], pgcommon['']], + c_args: ['-DFRONTEND'], + install: false, +) + +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 0000000000..4c56eb3a4b --- /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 0000000000..96cbd9c80e --- /dev/null +++ b/src/test/modules/test_cloexec/test_cloexec.c @@ -0,0 +1,267 @@ +/*------------------------------------------------------------------------- + * + * 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 +#include + +#ifdef WIN32 +#include +#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) + * - 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.46.0.windows.1