From 18bf94af6ac3f9df17c0ba4da168567c4fa6297d Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Wed, 24 Jun 2026 17:37:02 +0200 Subject: [PATCH] Fix possible startup loop with stale PID file after PID reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes an issue where, after an unclean exit, Webmin can leave `miniserv.pid` behind. If the kernel later reuses that PID for an unrelated process, the startup guard only checked that the PID was alive and refused to start with “Webmin is already running”. With systemd restart handling, this can leave Webmin permanently down until the PID file is manually removed. This change verifies that the live PID actually belongs to `miniserv.pl` running the same config before treating it as an active Webmin instance. On Linux, it reads `/proc//cmdline`, checks the miniserv script, and compares the config file by inode so symlinked paths still match and Usermin is correctly distinguished. If the PID is confirmed unrelated, the stale PID file is removed and startup continues. If the process cannot be inspected, the previous conservative behavior is preserved. Also hardens PID-file parsing with chomp and numeric validation, and adds tests for unrelated PID reuse, matching config, symlinked config, different miniserv config, and unreadable command-line fallback. --- miniserv.pl | 65 +++++++++++++++++++++++++++++++++++++++++++++++++--- t/miniserv.t | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/miniserv.pl b/miniserv.pl index 2133f9f3e..21b32b84d 100755 --- a/miniserv.pl +++ b/miniserv.pl @@ -143,9 +143,18 @@ if (-l $perl_path) { if (open(PIDFILE, $config{'pidfile'})) { my $already = ; close(PIDFILE); - chop($already); - if ($already && $already != $$ && kill(0, $already)) { - die "Webmin is already running with PID $already\n"; + chomp($already); + $already =~ s/^\s+|\s+$//g; + if ($already =~ /^\d+$/ && $already != $$ && kill(0, $already)) { + my $active = &pid_is_miniserv_for_config($already, $config_file); + if (defined($active) && !$active) { + warn "Ignoring stale Webmin PID file $config{'pidfile'} ". + "for unrelated PID $already\n"; + unlink($config{'pidfile'}); + } + else { + die "Webmin is already running with PID $already\n"; + } } } @@ -5182,6 +5191,56 @@ close($fh); return $rv; } +# read_process_cmdline(pid) +# Returns a process command line from /proc, or undef if unavailable. +sub read_process_cmdline +{ +my ($pid) = @_; +return undef if (!$pid || $pid !~ /^\d+$/); +return &read_any_file("/proc/$pid/cmdline"); +} + +# same_config_file(file1, file2) +# Returns 1 if two config paths refer to the same file. +sub same_config_file +{ +my ($file1, $file2) = @_; +return 0 if (!$file1 || !$file2); +return 1 if ($file1 eq $file2); +my @stat1 = stat($file1); +my @stat2 = stat($file2); +return @stat1 && @stat2 && $stat1[0] == $stat2[0] && + $stat1[1] == $stat2[1]; +} + +# pid_is_miniserv_for_config(pid, config-file) +# Returns 1 if pid appears to be miniserv for this config, 0 if it is a +# different process, or undef if this platform cannot tell. +sub pid_is_miniserv_for_config +{ +my ($pid, $config_file) = @_; +my $cmdline = &read_process_cmdline($pid); +return undef if (!defined($cmdline)); +return 0 if ($cmdline eq ""); +my @args = split(/\0/, $cmdline); +my $script_idx; +for(my $i=0; $i<@args; $i++) { + if ($args[$i] =~ /(?:^|\/)miniserv\.pl$/) { + $script_idx = $i; + last; + } + } +return 0 if (!defined($script_idx)); +my $config_arg; +for(my $i=$script_idx+1; $i<@args; $i++) { + next if ($args[$i] =~ /^-/); + $config_arg = $args[$i]; + last; + } +return defined($config_arg) ? + (&same_config_file($config_arg, $config_file) ? 1 : 0) : undef; +} + # update_vital_config() # Updates %config with defaults, and dies if something vital is missing sub update_vital_config diff --git a/t/miniserv.t b/t/miniserv.t index 59a690352..05c4fcd97 100644 --- a/t/miniserv.t +++ b/t/miniserv.t @@ -1282,6 +1282,70 @@ subtest 'read_any_file' => sub { 'open failure returns undef'); }; +# pid_is_miniserv_for_config — avoids stale pidfiles with reused PIDs +subtest 'pid_is_miniserv_for_config' => sub { + my $config = '/etc/webmin/miniserv.conf'; + my $pid = $$; + + { + no warnings 'redefine'; + local *miniserv::read_process_cmdline = sub { + return join("\0", '/usr/sbin/dovecot', '-F'); + }; + is(miniserv::pid_is_miniserv_for_config($pid, $config), 0, + 'live unrelated PID is not treated as active miniserv'); + } + + { + no warnings 'redefine'; + local *miniserv::read_process_cmdline = sub { + return join("\0", '/usr/bin/perl', + '/usr/libexec/webmin/miniserv.pl', + '--nofork', $config); + }; + is(miniserv::pid_is_miniserv_for_config($pid, $config), 1, + 'matching miniserv config is treated as active'); + } + + { + require File::Temp; + my ($fh, $real_config) = File::Temp::tempfile(UNLINK => 1); + close($fh); + my $link_config = "$real_config.link"; + SKIP: { + symlink($real_config, $link_config) || + skip 'symlinks are unavailable', 1; + no warnings 'redefine'; + local *miniserv::read_process_cmdline = sub { + return join("\0", '/usr/bin/perl', + '/usr/libexec/webmin/miniserv.pl', + $link_config); + }; + is(miniserv::pid_is_miniserv_for_config($pid, $real_config), 1, + 'symlinked config path is matched by inode'); + unlink($link_config); + } + } + + { + no warnings 'redefine'; + local *miniserv::read_process_cmdline = sub { + return join("\0", '/usr/bin/perl', + '/usr/libexec/usermin/miniserv.pl', + '--nofork', '/etc/usermin/miniserv.conf'); + }; + is(miniserv::pid_is_miniserv_for_config($pid, $config), 0, + 'different miniserv config is not treated as active'); + } + + { + no warnings 'redefine'; + local *miniserv::read_process_cmdline = sub { return undef; }; + is(miniserv::pid_is_miniserv_for_config($pid, $config), undef, + 'unreadable process command line keeps conservative behavior'); + } +}; + # read_mime_types — populates %mime from file + addtype_* config subtest 'read_mime_types' => sub { no warnings 'once';