From 34336186f80c4770efad94aa595d7ec32ee9bd36 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Sat, 12 Sep 2026 19:13:10 +0200 Subject: [PATCH] Add WebminCron pause during module post-install This PR stops Miniserv from starting scheduled jobs while run-postinstalls.pl is running, so jobs never execute against half-updated module code or config. Miniserv defers all jobs, including startup jobs, while any marker belongs to a live process, and discards markers left by runners that died or that are older than an hour. The pause is best-effort, as a failure to create the marker is reported but never blocks post-installs. By the way, this issue was not reported by anyone. I discovered it while running many other extensive tests. --- miniserv-lib.pl | 42 +++++++++++++- miniserv.pl | 2 +- run-postinstalls.pl | 38 +++++++++++- t/miniserv.t | 119 ++++++++++++++++++++++++++++++++++++++ t/run-postinstalls.t | 134 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 331 insertions(+), 4 deletions(-) create mode 100644 t/run-postinstalls.t diff --git a/miniserv-lib.pl b/miniserv-lib.pl index 42335b6f7..d93053d07 100644 --- a/miniserv-lib.pl +++ b/miniserv-lib.pl @@ -10,6 +10,10 @@ eval "use Time::HiRes;"; @itoa64 = split(//, "./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"); @miniserv_argv = @ARGV; +# Limit scheduler pauses so an interrupted update or reused PID cannot block +# WebminCron jobs indefinitely. +$webmincron_pause_max_age = 60*60; + # init days and months for http_date @weekday = ( "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" ); @month = ( "Jan", "Feb", "Mar", "Apr", "May", "Jun", @@ -3975,6 +3979,7 @@ if (!$config{'webmincron_wrapper'}) { $config{'webmincron_wrapper'} = $config{'root'}. "/webmincron/webmincron.pl"; } +$config{'webmincron_pause'} ||= $var_dir."/webmincron-pause"; if (!$config{'twofactor_wrapper'}) { $config{'twofactor_wrapper'} = $config{'root'}."/acl/twofactor.pl"; } @@ -5833,12 +5838,44 @@ foreach $k (keys %{$_[1]}) { close(ARFILE); } +# webmincrons_paused() +# Returns true while an unexpired marker has a live PID. +# Removes expired markers and markers for dead PIDs. +sub webmincrons_paused +{ +my $pause_dir = $config{'webmincron_pause'}.".d"; +opendir(my $pause, $pause_dir) || return 0; +my $paused = 0; +my $now = time(); +foreach my $marker (readdir($pause)) { + next if ($marker !~ /^(\d+)$/); + my $pid = $1; + my $file = "$pause_dir/$marker"; + my @st = stat($file); + if (@st && $now - $st[9] > $webmincron_pause_max_age) { + # Enforce the maximum pause even if the PID remains live or has + # been reused. + unlink($file); + } + # EPERM means the PID exists but Miniserv cannot signal it. + elsif (kill(0, $pid) || $!{'EPERM'}) { + $paused = 1; + } + else { + unlink($file); + } + } +closedir($pause); +return $paused; +} + # execute_ready_webmin_crons(run-count) -# Find and run any cron jobs that are due, based on their last run time and -# execution interval +# Runs due WebminCron jobs. Returns false while paused, preserving the startup +# pass for later. sub execute_ready_webmin_crons { my ($runs) = @_; +return 0 if (&webmincrons_paused()); my $now = time(); my $changed = 0; foreach my $cron (@webmincrons) { @@ -5890,6 +5927,7 @@ if ($changed) { # Write out file containing last run times &write_file($config{'webmincron_last'}, \%webmincron_last); } +return 1; } # matches_cron(cron-spec, time, first-value) diff --git a/miniserv.pl b/miniserv.pl index 3ad64bef7..cd362fab4 100755 --- a/miniserv.pl +++ b/miniserv.pl @@ -631,7 +631,7 @@ while(1) { } # Check if any webmin cron jobs are ready to run - &execute_ready_webmin_crons($cron_runs++); + $cron_runs++ if (&execute_ready_webmin_crons($cron_runs)); # wait for a new connection, or a message from a child process local ($i, $rmask); diff --git a/run-postinstalls.pl b/run-postinstalls.pl index aa86378d2..3f177317f 100755 --- a/run-postinstalls.pl +++ b/run-postinstalls.pl @@ -9,6 +9,43 @@ use WebminCore; &init_config(); $current_theme = $WebminCore::current_theme = undef; +# Pause WebminCron jobs while module post-install scripts run. +# Each runner uses its own marker. +my %miniserv; +&get_miniserv_config(\%miniserv); +# Use the same default path as Miniserv. +my $miniserv_var_dir = $var_directory; +$miniserv_var_dir = $1 + if ($miniserv{'pidfile'} && + $miniserv{'pidfile'} =~ /^(.*)\/[^\/]+$/); +my $webmincron_pause_file = $miniserv{'webmincron_pause'} || + $miniserv_var_dir."/webmincron-pause"; +my $webmincron_pause_dir = $webmincron_pause_file.".d"; +my $webmincron_pause_marker = $webmincron_pause_dir."/".$$; +my $webmincron_pause_pid = $$; +my $webmincron_paused; +if (!-d $webmincron_pause_dir && + !mkdir($webmincron_pause_dir, 0700) && + !-d $webmincron_pause_dir) { + print STDERR "Cannot pause scheduled jobs: failed to create " . + "$webmincron_pause_dir: $!\n"; + } +elsif (open(my $pause, ">", $webmincron_pause_marker)) { + close($pause); + $webmincron_paused = 1; + } +else { + print STDERR "Cannot pause scheduled jobs: failed to create " . + "$webmincron_pause_marker: $!\n"; + } +# Forked children also run the END block, so only the owner removes its marker. +# Keep the shared directory for concurrent runners. +END { + if ($webmincron_paused && $$ == $webmincron_pause_pid) { + unlink($webmincron_pause_marker); + } + } + if (@ARGV > 0) { # Running for specified modules foreach my $a (@ARGV) { @@ -37,4 +74,3 @@ foreach my $m (@mods) { } } } - diff --git a/t/miniserv.t b/t/miniserv.t index 12ccd2760..7a6f96bb6 100644 --- a/t/miniserv.t +++ b/t/miniserv.t @@ -251,6 +251,125 @@ subtest 'prefix_to_mask' => sub { is(miniserv::prefix_to_mask(32), '255.255.255.255', '/32 = all ones'); }; +# webmincrons_paused — post-install pause markers +subtest 'webmincrons_paused' => sub { + my $pause = File::Spec->catfile(File::Spec->tmpdir(), + "webmincron-pause-$$"); + my $pause_dir = $pause.'.d'; + my $default_pause = File::Spec->catfile(File::Spec->tmpdir(), + "webmincron-pause"); + { + no warnings 'once'; + local %miniserv::config = ( + 'pidfile' => File::Spec->catfile(File::Spec->tmpdir(), + "miniserv-$$.pid"), + 'logfile' => File::Spec->catfile(File::Spec->tmpdir(), + "miniserv-$$.log"), + ); + local $miniserv::config_file = File::Spec->catfile( + File::Spec->tmpdir(), "miniserv-$$.conf"); + miniserv::update_vital_config(); + is($miniserv::config{'webmincron_pause'}, $default_pause, + 'pause path defaults to the Miniserv variable directory'); + } + local $miniserv::config{'webmincron_pause'} = $pause; + + rmdir($pause_dir); + ok(!miniserv::webmincrons_paused(), + 'scheduler is not paused without markers'); + + mkdir($pause_dir, 0700) or die "create pause directory: $!"; + my $live_marker = File::Spec->catfile($pause_dir, $$); + open(my $live, '>', $live_marker) or die "open pause marker: $!"; + close($live) or die "close pause marker: $!"; + my $stale_marker = File::Spec->catfile($pause_dir, 2147483647); + open(my $stale, '>', $stale_marker) or die "open pause marker: $!"; + close($stale) or die "close pause marker: $!"; + ok(miniserv::webmincrons_paused(), + 'live marker pauses scheduled jobs'); + ok(!-e $stale_marker, 'stale markers are removed while jobs are paused'); + + unlink($live_marker); + open($stale, '>', $stale_marker) or die "open pause marker: $!"; + close($stale) or die "close pause marker: $!"; + ok(!miniserv::webmincrons_paused(), + 'stale pause marker is ignored'); + ok(!-e $stale_marker, 'stale pause marker is removed'); + + # A marker past the age limit is stale even though its PID is alive. + { + no warnings 'once'; + open($live, '>', $live_marker) or die "open pause marker: $!"; + close($live) or die "close pause marker: $!"; + my $old = time() - $miniserv::webmincron_pause_max_age - 1; + utime($old, $old, $live_marker) or die "age pause marker: $!"; + ok(!miniserv::webmincrons_paused(), + 'expired pause marker is ignored'); + ok(!-e $live_marker, 'expired pause marker is removed'); + } + + # Pausing delays startup jobs instead of skipping them. + my $last = $pause.'-last'; + my $started = 0; + { + no warnings qw(once redefine); + local @miniserv::webmincrons = ({ + 'id' => 'test', 'boot' => 1, 'module' => 'test', + 'func' => 'test', + }); + local %miniserv::webmincron_last = (); + local @miniserv::childpids = (); + local $miniserv::config{'webmincron_last'} = $last; + local *miniserv::execute_webmin_command = sub { + $started++; + return 12345; + }; + + open(my $active, '>', $live_marker) + or die "open pause marker: $!"; + close($active) or die "close pause marker: $!"; + ok(!miniserv::execute_ready_webmin_crons(0), + 'scheduler defers jobs while paused'); + is($started, 0, 'paused scheduler starts no jobs'); + is_deeply(\%miniserv::webmincron_last, {}, + 'paused scheduler preserves job history'); + ok(!-e $last, 'paused scheduler does not write job history'); + + unlink($live_marker); + ok(miniserv::execute_ready_webmin_crons(0), + 'scheduler resumes after marker removal'); + is($started, 1, 'deferred startup job runs after marker removal'); + ok(miniserv::execute_ready_webmin_crons(1), + 'subsequent scheduler passes complete'); + is($started, 1, 'startup job does not run again'); + + # Overdue interval jobs retain their due time until the pause ends. + local @miniserv::webmincrons = ({ + 'id' => 'interval', 'interval' => 60, 'module' => 'test', + 'func' => 'test', + }); + my $previous = time() - 120; + local %miniserv::webmincron_last = ('interval' => $previous); + open($active, '>', $live_marker) + or die "open pause marker: $!"; + close($active) or die "close pause marker: $!"; + ok(!miniserv::execute_ready_webmin_crons(1), + 'scheduler defers overdue interval jobs while paused'); + is($started, 1, 'paused interval job does not start'); + is($miniserv::webmincron_last{'interval'}, $previous, + 'paused interval job retains its last run time'); + unlink($live_marker); + ok(miniserv::execute_ready_webmin_crons(1), + 'scheduler resumes overdue interval jobs'); + is($started, 2, 'overdue interval job runs after marker removal'); + } + + unlink($live_marker); + unlink($stale_marker); + rmdir($pause_dir); + unlink($last); +}; + # check_ipaddress / check_ip6address — input validators subtest 'check_ipaddress' => sub { ok( miniserv::check_ipaddress('1.2.3.4'), 'valid IPv4 accepted'); diff --git a/t/run-postinstalls.t b/t/run-postinstalls.t new file mode 100644 index 000000000..34b1318f0 --- /dev/null +++ b/t/run-postinstalls.t @@ -0,0 +1,134 @@ +#!/usr/bin/perl +# Tests scheduler pause markers around module post-install scripts. + +use strict; +use warnings; +use Test::More; +use Cwd qw(abs_path); +use File::Basename qw(dirname); +use File::Path qw(make_path); +use File::Spec; +use File::Temp qw(tempdir); +use IPC::Open3; +use Symbol qw(gensym); + +my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..')); +my $runner = File::Spec->catfile($root, 'run-postinstalls.pl'); +my $tmp = tempdir(CLEANUP => 1); +my $config_dir = File::Spec->catdir($tmp, 'config'); +my $var_dir = File::Spec->catdir($tmp, 'var'); +my $extra_root = File::Spec->catdir($tmp, 'root'); +my $module_dir = File::Spec->catdir($extra_root, 'pause-test'); +my $sentinel = File::Spec->catfile($tmp, 'postinstall-ran'); +make_path($config_dir, $var_dir, $module_dir); + +write_text(File::Spec->catfile($config_dir, 'config'), ""); +write_text(File::Spec->catfile($module_dir, 'module.info'), + "name=Pause test\ndesc=Pause test\n"); +write_text(File::Spec->catfile($module_dir, 'postinstall.pl'), <<'EOF'); +sub module_install +{ +# A child that exits without exec also runs the runner's END block. It must not +# remove the parent's marker. +my $child = fork(); +die "fork: $!" if (!defined($child)); +exit(0) if (!$child); +waitpid($child, 0); +open(my $sentinel, ">", $ENV{'POSTINSTALL_SENTINEL'}) || + die "create post-install sentinel: $!"; +my $marker = $ENV{'POSTINSTALL_PAUSE'}.".d/".$$; +my $state = -e $marker ? "paused\n" : "unpaused\n"; +print $sentinel $state; +close($sentinel) || die "close post-install sentinel: $!"; +} +1; +EOF + +sub write_text +{ +my ($file, $text) = @_; +open(my $fh, '>', $file) || die "create $file: $!"; +print $fh $text; +close($fh) || die "close $file: $!"; +} + +sub run_postinstalls +{ +my ($pause, $set_pause) = @_; +my $pause_config = $set_pause ? "webmincron_pause=$pause\n" : ""; +write_text(File::Spec->catfile($config_dir, 'miniserv.conf'), + "root=$root\n". + "extraroot_0=$extra_root\n". + "pidfile=$var_dir/miniserv.pid\n". + "logfile=$var_dir/miniserv.log\n". + $pause_config); +unlink($sentinel); + +local $ENV{'WEBMIN_CONFIG'} = $config_dir; +local $ENV{'WEBMIN_VAR'} = $var_dir; +local $ENV{'POSTINSTALL_SENTINEL'} = $sentinel; +local $ENV{'POSTINSTALL_PAUSE'} = $pause; +my $stderr = gensym(); +my $pid = open3(undef, my $stdout, $stderr, $^X, $runner, 'pause-test'); +local $/; +my $out = <$stdout> // ''; +my $err = <$stderr> // ''; +waitpid($pid, 0); +return ($? >> 8, $out, $err); +} + +my $pause = File::Spec->catfile($var_dir, 'webmincron-pause'); +my ($status, $output, $error) = run_postinstalls($pause, 0); +is($status, 0, 'post-install runner exits successfully'); +is(read_text($sentinel), "paused\n", + 'forked child does not remove the runner\'s marker'); +ok(-d $pause.'.d', 'pause directory remains after the runner exits'); +is_deeply([ numeric_markers($pause.'.d') ], [], + 'post-install marker is removed on exit'); +is($error, '', 'successful pause setup reports no error'); + +# A runner using a custom path must leave another runner's marker intact. +my $custom_pause = File::Spec->catfile($var_dir, 'custom-pause'); +my $custom_dir = $custom_pause.'.d'; +make_path($custom_dir); +my $other_marker = File::Spec->catfile($custom_dir, $$); +write_text($other_marker, ""); +($status, $output, $error) = run_postinstalls($custom_pause, 1); +is($status, 0, 'runner succeeds with a configured pause path'); +is(read_text($sentinel), "paused\n", 'runner uses the configured pause path'); +is_deeply([ numeric_markers($custom_dir) ], [ "$$" ], + 'runner removes only its own marker'); +is($error, '', 'overlapping pause markers report no error'); +unlink($other_marker); + +# Failure to create the shared directory must not skip post-install scripts. +my $blocker = File::Spec->catfile($tmp, 'not-a-directory'); +write_text($blocker, ""); +my $invalid_pause = File::Spec->catfile($blocker, 'webmincron-pause'); +($status, $output, $error) = run_postinstalls($invalid_pause, 1); +is($status, 0, 'pause setup failure does not stop the runner'); +is(read_text($sentinel), "unpaused\n", + 'module post-install still runs when pause setup fails'); +like($error, qr/^Cannot pause scheduled jobs:/m, + 'pause setup failure is reported'); + +sub read_text +{ +my ($file) = @_; +open(my $fh, '<', $file) || die "open $file: $!"; +local $/; +my $text = <$fh>; +close($fh); +return $text; +} + +sub numeric_markers +{ +my ($dir) = @_; +opendir(my $dh, $dir) || die "open $dir: $!"; +my @markers = grep { /^\d+$/ } readdir($dh); +closedir($dh); +return @markers; +} + +done_testing();