From 0004ca9411992b853af2ac0749191e84b56a1565 Mon Sep 17 00:00:00 2001 From: Harrison Booth Date: Tue, 20 Oct 2026 14:10:16 -0500 Subject: [PATCH] Add bounded Windows socket inheritance regression coverage Regression and test-registration supplement to Bryan Green's original socket inheritance proposal, which must be applied first and retains its original authorship and mail bytes. Production proposal remains Bryan's. Use the existing recorded local v5 test/helper registration representation. The ordered series remains source-equivalent to the exact hosted combined v5 input; do not apply that combined input as an additional patch. Force a fresh initdb for this TAP test so it does not rely on the installed initdb template's file attributes when pg_regress configures authentication. Portable reviewer adaptation and Job-aware joint coverage remain separate. --- src/bin/pg_ctl/Makefile | 11 +- src/bin/pg_ctl/meson.build | 10 + .../pg_ctl/t/005_socket_handle_inheritance.pl | 236 ++++++++++-------- src/bin/pg_ctl/t/meson.build | 10 + src/bin/pg_ctl/t/socket_copy_helper.c | 79 ++++++ 5 files changed, 236 insertions(+), 110 deletions(-) create mode 100644 src/bin/pg_ctl/t/meson.build create mode 100644 src/bin/pg_ctl/t/socket_copy_helper.c diff --git a/src/bin/pg_ctl/Makefile b/src/bin/pg_ctl/Makefile index 5c2d418..2099161 100644 --- a/src/bin/pg_ctl/Makefile +++ b/src/bin/pg_ctl/Makefile @@ -43,7 +43,7 @@ uninstall: rm -f '$(DESTDIR)$(bindir)/pg_ctl$(X)' clean distclean: - rm -f pg_ctl$(X) $(OBJS) + rm -f pg_ctl$(X) $(OBJS) pg_ctl_socket_helper$(X) rm -rf tmp_check check: @@ -51,3 +51,12 @@ check: installcheck: $(prove_installcheck) + +ifeq ($(PORTNAME), win32) +pg_ctl_socket_helper$(X): t/socket_copy_helper.c + $(CC) $(CFLAGS) $(CPPFLAGS) $< $(LDFLAGS) $(LIBS) -o $@ + +check: pg_ctl_socket_helper$(X) +installcheck: pg_ctl_socket_helper$(X) +export PG_CTL_SOCKET_HELPER := $(abspath pg_ctl_socket_helper$(X)) +endif diff --git a/src/bin/pg_ctl/meson.build b/src/bin/pg_ctl/meson.build index 3e64681..4f0e677 100644 --- a/src/bin/pg_ctl/meson.build +++ b/src/bin/pg_ctl/meson.build @@ -17,11 +17,21 @@ pg_ctl = executable('pg_ctl', ) bin_targets += pg_ctl +socket_test_deps = [] +socket_test_env = {} +if host_system == 'windows' + subdir('t') + socket_test_deps += socket_test_helper + socket_test_env = {'PG_CTL_SOCKET_HELPER': socket_test_helper.full_path()} +endif + tests += { 'name': 'pg_ctl', 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), 'tap': { + 'deps': socket_test_deps, + 'env': socket_test_env, 'tests': [ 't/001_start_stop.pl', 't/002_status.pl', diff --git a/src/bin/pg_ctl/t/005_socket_handle_inheritance.pl b/src/bin/pg_ctl/t/005_socket_handle_inheritance.pl index 63e1955..3c12c05 100644 --- a/src/bin/pg_ctl/t/005_socket_handle_inheritance.pl +++ b/src/bin/pg_ctl/t/005_socket_handle_inheritance.pl @@ -1,134 +1,152 @@ -# Copyright (c) 2025, PostgreSQL Global Development Group - -# Test that socket handles are not inherited by child processes on Windows. -# -# Without the fix, child processes spawned via COPY TO PROGRAM inherit socket -# handles from the backend. Windows reference counting prevents these sockets -# from being freed when the postmaster exits, leaving the port bound to a dead -# process (a "zombie" binding). This test verifies that after killing the -# postmaster while a child process is still running, the listening port is -# immediately freed rather than remaining in a zombie state. - +# Copyright (c) 2026, PostgreSQL Global Development Group use strict; -use warnings; - +use warnings FATAL => 'all'; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; -use Time::HiRes qw(sleep); - -# This test is Windows-specific -if ($^O ne 'MSWin32') +use IO::Socket::INET; +use IO::Select; +use IPC::Run qw(run); +use Time::HiRes qw(time sleep); + +plan skip_all => 'Windows socket inheritance test' unless $windows_os; +my $helper = $ENV{PG_CTL_SOCKET_HELPER}; +die 'PG_CTL_SOCKET_HELPER is required' unless defined $helper && -f $helper; +$helper =~ s!\\!/!g; +my $node = PostgreSQL::Test::Cluster->new('socket_inheritance'); +my $dir = PostgreSQL::Test::Utils::tempdir; +my $ready = "$dir/copy.ready"; +my $release = "$dir/copy.release"; +my (@identity, $socket); + +sub helper_exit { - plan skip_all => 'test is specific to Windows socket handle inheritance'; + my (@args) = @_; + my ($stdout, $stderr); + run [ $helper, @args ], '>', \$stdout, '2>', \$stderr; + return $? >> 8; } -my $node = PostgreSQL::Test::Cluster->new('main'); -$node->init; -$node->start; - -# Get the port number for verification -my $port = $node->port; - -# Spawn a long-running child process via COPY TO PROGRAM that will outlive -# the postmaster. Without the fix, this child inherits socket handles. -my $marker_file = $node->data_dir . '/ps_marker.txt'; -unlink $marker_file if -e $marker_file; - -$node->safe_psql( - 'postgres', - qq{\\copy (select 1) to program 'powershell -Command "echo marker > $marker_file; Start-Sleep 120"'} -); - -# Wait for PowerShell to spawn -my $ps_spawned = 0; -for (my $i = 0; $i < 100; $i++) +# Release only our bounded helper. Cluster cleanup knows the owned postmaster. +END { - if (-e $marker_file) + if (defined $release && -d $dir) { - $ps_spawned = 1; - last; + open my $f, '>', $release or die $!; + close $f; } - sleep 0.1; + helper_exit('--wait', @identity) if @identity; + close $socket if defined $socket; + $node->stop('immediate') if defined $node; } -ok($ps_spawned, 'child process spawned successfully'); - -# Stop the postmaster (simulates a crash), leaving the child process running. -$node->stop('immediate'); -sleep 0.5; - -# Verify that the listening port is freed immediately. With the bug, the port -# remains bound to the dead postmaster PID because the child process inherited -# the socket handles. With the fix, the port is freed because socket handles -# were not inherited. -my $netstat_output = `netstat -ano | findstr ":$port.*LISTENING"`; - -if ($netstat_output) +sub read_exact { - fail('listening port remains bound after postmaster exit (zombie port)'); - diag("Port is still bound - socket handles were inherited by child process"); - diag("netstat output:\n$netstat_output"); - - if ($netstat_output =~ /LISTENING\s+(\d+)/) + my ($count) = @_; + my $bytes = ''; + while (length($bytes) < $count) { - my $bound_pid = $1; - my $process_name = get_process_name($bound_pid); - - if ($process_name eq 'unknown' || $process_name eq '') - { - diag("Port bound to dead process (PID $bound_pid) - zombie binding detected"); - } - else - { - diag("Port bound to: $process_name (PID $bound_pid)"); - } + die 'startup socket read timed out' + unless IO::Select->new($socket)->can_read(10); + my $n = sysread($socket, my $part, $count - length($bytes)); + die "startup socket read: $!" unless defined $n && $n; + $bytes .= $part; } + return $bytes; } -else + +sub write_packet { - pass('listening port freed immediately after postmaster exit'); + my ($bytes) = @_; + while (length($bytes)) + { + my $n = syswrite($socket, $bytes); + die "socket write: $!" unless defined $n && $n; + substr($bytes, 0, $n, ''); + } } -# Additional verification: Confirm the port is actually available for binding. -# This tests the real-world scenario that matters to users. -my $can_bind = test_port_available($port); -ok($can_bind, "port $port is available for new connections"); - -# Cleanup -cleanup_powershell_processes(); -unlink $marker_file if -e $marker_file; - -done_testing(); - -# Test if port can actually be bound -sub test_port_available +sub transport_closed { - my ($port) = @_; - - use Socket; - - socket(my $sock, PF_INET, SOCK_STREAM, getprotobyname('tcp')) or return 0; - setsockopt($sock, SOL_SOCKET, SO_REUSEADDR, 1); - - my $addr = sockaddr_in($port, INADDR_ANY); - my $result = bind($sock, $addr); - close($sock); - - return $result ? 1 : 0; + my $deadline = time + 1; + my $select = IO::Select->new($socket); + while (time < $deadline) + { + return 0 unless $select->can_read($deadline - time); + my $n = sysread($socket, my $bytes, 65536); + return 1 if defined $n && $n == 0; + return 1 if !defined $n && $!{ECONNRESET}; + die "socket read: $!" unless defined $n; + } + return 0; } -# Get process name by PID -sub get_process_name +$node->init(force_initdb => 1); +$node->append_conf('postgresql.conf', "listen_addresses = '127.0.0.1'"); +# The raw test client does not implement the default Windows SSPI exchange. +# Change only this disposable node's loopback authentication rule. +my $hba = $node->data_dir . '/pg_hba.conf'; +my $hba_contents = slurp_file($hba); +open my $auth, '>', $hba or die $!; +print $auth "host all all 127.0.0.1/32 trust\n", $hba_contents; +close $auth; +$node->start; +$socket = IO::Socket::INET->new(PeerAddr => '127.0.0.1', + PeerPort => $node->port, Proto => 'tcp', Timeout => 10) or die $!; +my $user = $node->safe_psql('postgres', 'SELECT current_user'); +$user =~ s/[\r\n]+\z//; +my $body = pack('N', 196608) . "user\0$user\0database\0postgres\0\0"; +write_packet(pack('N', length($body) + 4) . $body); +while (1) { - my ($pid) = @_; - my $name = `powershell -Command "(Get-Process -Id $pid -ErrorAction SilentlyContinue).ProcessName" 2>nul`; - chomp $name; - return $name || 'unknown'; + my $type = read_exact(1); + my $length = unpack('N', read_exact(4)); + my $message = read_exact($length - 4); + die "startup error: $message" if $type eq 'E'; + die 'raw client requires trust authentication' + if $type eq 'R' && unpack('N', $message) != 0; + last if $type eq 'Z'; } - -# Clean up test child processes -sub cleanup_powershell_processes +my $program = qq("$helper" "$ready" "$release"); +$program =~ s/'/''/g; +my $query = "COPY (SELECT 1) TO PROGRAM '$program'\0"; +write_packet('Q' . pack('N', length($query) + 4) . $query); +my $deadline = time + 10; +while (time < $deadline) { - system('powershell -Command "Get-Process powershell -ErrorAction SilentlyContinue | Where-Object {$_.Id -ne $PID} | Stop-Process -Force -ErrorAction SilentlyContinue" 2>nul'); -} \ No newline at end of file + if (-f $ready) + { + my $identity = slurp_file($ready); + @identity = ($identity =~ /^(\d+) (\d+) (\d+)\n$/); + last if @identity; + } + sleep 0.05; +} +ok(scalar(@identity) == 3, 'COPY helper publishes exact PID and creation time'); +die 'helper did not become ready' unless @identity; +is(helper_exit('--alive', @identity), 0, 'helper alive before shutdown'); +$node->stop('immediate'); +ok(!-f $node->data_dir . '/postmaster.pid', 'owned postmaster stopped'); +my $survived = helper_exit('--alive', @identity) == 0; +ok($survived, 'precondition: COPY helper remains alive after shutdown'); +SKIP: +{ + skip 'environment terminated the COPY helper, socket lifetime not tested', 2 + unless $survived; + ok(transport_closed(), 'backend client closes while COPY helper is alive'); + is(helper_exit('--bind', $node->port), 0, + 'listener port can be rebound while COPY helper is alive'); +} +append_to_file($release, 'release'); +is(helper_exit('--wait', @identity), 0, 'exact helper exits after release'); +$deadline = time + 5; +my $bound = 1; +while (time < $deadline) +{ + $bound = helper_exit('--bind', $node->port); + last if $bound == 0; + sleep 0.05; +} +is($bound, 0, 'port reusable after helper and COPY shell exit'); +ok(transport_closed(), 'client transport closed after helper release'); +done_testing(); diff --git a/src/bin/pg_ctl/t/meson.build b/src/bin/pg_ctl/t/meson.build new file mode 100644 index 0000000..622deb7 --- /dev/null +++ b/src/bin/pg_ctl/t/meson.build @@ -0,0 +1,10 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Keep the test helper out of pg_ctl's build directory. Meson adds test +# dependency directories to PATH, and the uninstalled pg_ctl cannot find +# its sibling postgres executable there. +socket_test_helper = executable('pg_ctl_socket_helper', + 'socket_copy_helper.c', + dependencies: cc.find_library('ws2_32'), + install: false, +) diff --git a/src/bin/pg_ctl/t/socket_copy_helper.c b/src/bin/pg_ctl/t/socket_copy_helper.c new file mode 100644 index 0000000..79fac3c --- /dev/null +++ b/src/bin/pg_ctl/t/socket_copy_helper.c @@ -0,0 +1,79 @@ +/* Copyright (c) 2026, PostgreSQL Global Development Group */ +#include +#include +#include +#include +#include + +/* Private test helper: identify exactly one process, wait, or probe a port. */ +int +main(int argc, char **argv) +{ + if (argc == 3 && strcmp(argv[1], "--bind") == 0) + { + WSADATA data; + SOCKET s; + struct sockaddr_in address = {0}; + BOOL exclusive = TRUE; + int result; + if (WSAStartup(MAKEWORD(2, 2), &data) != 0) + return 2; + s = socket(AF_INET, SOCK_STREAM, 0); + if (s == INVALID_SOCKET || setsockopt(s, SOL_SOCKET, SO_EXCLUSIVEADDRUSE, + (const char *) &exclusive, sizeof(exclusive)) != 0) + return 2; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + address.sin_port = htons((unsigned short) atoi(argv[2])); + result = bind(s, (struct sockaddr *) &address, sizeof(address)); + closesocket(s); + WSACleanup(); + return result == 0 ? 0 : 1; + } + if (argc == 5 && (strcmp(argv[1], "--alive") == 0 || + strcmp(argv[1], "--wait") == 0)) + { + HANDLE process; + FILETIME created, exited, kernel, user; + DWORD result; + BOOL wait = strcmp(argv[1], "--wait") == 0; + process = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_LIMITED_INFORMATION, + FALSE, strtoul(argv[2], NULL, 10)); + if (process == NULL) + return GetLastError() == ERROR_INVALID_PARAMETER && wait ? 0 : 2; + if (!GetProcessTimes(process, &created, &exited, &kernel, &user)) + { + CloseHandle(process); + return 2; + } + /* A reused PID must never count as the owned helper. */ + if (created.dwHighDateTime != strtoul(argv[3], NULL, 10) || + created.dwLowDateTime != strtoul(argv[4], NULL, 10)) + { + CloseHandle(process); + return wait ? 0 : 1; + } + result = WaitForSingleObject(process, wait ? 10000 : 0); + CloseHandle(process); + return result == (wait ? WAIT_OBJECT_0 : WAIT_TIMEOUT) ? 0 : 1; + } + if (argc == 3) + { + FILE *ready; + FILETIME created, exited, kernel, user; + DWORD deadline = GetTickCount() + 35000; + if (!GetProcessTimes(GetCurrentProcess(), &created, &exited, &kernel, &user)) + return 2; + ready = fopen(argv[1], "w"); + if (ready == NULL) + return 2; + fprintf(ready, "%lu %lu %lu\n", GetCurrentProcessId(), + created.dwHighDateTime, created.dwLowDateTime); + fclose(ready); + while (GetFileAttributes(argv[2]) == INVALID_FILE_ATTRIBUTES && + (LONG) (deadline - GetTickCount()) > 0) + Sleep(50); + return 0; + } + return 2; +}