Fix O_CLOEXEC flag handling in Windows port.

macdice · macdice · commit f24af0e04cef · 2025-12-10T09:10:31.000+13:00
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 1da569c (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 <dbryan.green@gmail.com> Co-authored-by: Thomas Munro <thomas.munro@gmail.com> (minor adjustments) Discussion: https://postgr.es/m/e2b16375-7430-4053-bda3-5d2194ff1880%40gmail.com
diff --git a/src/include/port.h b/src/include/port.h
@@ -338,7 +338,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
@@ -343,18 +343,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	0x04000000
+#define	O_DIRECT	0x80000000
+#define	O_DSYNC		_O_NOINHERIT
 
 /*
  * Supplement to <errno.h>.
diff --git 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
@@ -14,6 +14,7 @@ SUBDIRS = \
 		  plsample \
 		  spgist_name_ops \
 		  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
@@ -13,6 +13,7 @@ subdir('plsample')
 subdir('spgist_name_ops')
 subdir('ssl_passphrase_callback')
 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
@@ -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
@@ -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
@@ -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();
diff --git a/src/test/modules/test_cloexec/test_cloexec.c b/src/test/modules/test_cloexec/test_cloexec.c
