From f206cbb23a1f2e9bd623b61b2619e72f10347775 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Mon, 14 Sep 2026 21:54:26 +0200 Subject: [PATCH 1/7] Fix concurrent Nginx configuration updates This PR fixes concurrent Nginx updates using stale config data. It reloads the config after locking and safely handles nested locks and included files. --- nginx/nginx-lib.pl | 47 ++++++++++- nginx/t/config-locks.t | 183 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 nginx/t/config-locks.t diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index f69b079d1..a1afe3370 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -18,6 +18,7 @@ $last_config_change_flag = $module_var_directory."/config-flag"; $last_restart_time_flag = $module_var_directory."/restart-flag"; my @lock_all_config_files_cache; +my $lock_all_config_files_depth = 0; # set_nginx_config_defaults() # Fill in sensible defaults if module config has not been initialized yet @@ -534,14 +535,50 @@ foreach my $f (@files) { } # lock_all_config_files([&parent]) -# Locks all files used in the current config +# Locks all config files and refreshes the config on the outermost call. +# Fetch directive objects after locking; earlier objects may have stale lines. sub lock_all_config_files { my ($parent) = @_; -@lock_all_config_files_cache = &get_all_config_files($parent); -foreach my $f (@lock_all_config_files_cache) { - &lock_file($f); +if ($lock_all_config_files_depth) { + # Nested edits share the caller's config tree and pending changes. + $lock_all_config_files_depth++; + return; } + +# Lock the main file before parsing, so another writer cannot change the +# include list or directive line numbers while we acquire the remaining locks. +my $main = &resolve_links($config{'nginx_config'}) || $config{'nginx_config'}; +my $ok = eval { + my @files = ($main); + my %locked; + while (@files) { + foreach my $f (@files) { + if (&lock_file($f)) { + push(@lock_all_config_files_cache, $f); + } + elsif (!defined($main::locked_file_list{&translate_filename($f)})) { + &error("Failed to lock Nginx config file $f"); + } + $locked{$f} = 1; + &unflush_file_lines($f); + } + # An included file may also be edited directly. Re-read after + # waiting for its lock and pick up any newly included files. + &flush_config_cache(); + @files = grep { !$locked{$_} } &unique(&get_all_config_files(), + $parent ? &get_all_config_files($parent) : ()); + } + 1; + }; +my $err = $@; +if (!$ok) { + # Do not leave partially acquired locks behind if parsing or locking fails. + &unlock_file($_) foreach reverse(@lock_all_config_files_cache); + @lock_all_config_files_cache = (); + die $err; + } +$lock_all_config_files_depth = 1; } # unlock_all_config_files([&parent]) @@ -549,6 +586,8 @@ foreach my $f (@lock_all_config_files_cache) { sub unlock_all_config_files { my ($parent) = @_; +return if (!$lock_all_config_files_depth); +return if (--$lock_all_config_files_depth); foreach my $f (reverse(@lock_all_config_files_cache)) { &unlock_file($f); } diff --git a/nginx/t/config-locks.t b/nginx/t/config-locks.t new file mode 100644 index 000000000..f27e49e33 --- /dev/null +++ b/nginx/t/config-locks.t @@ -0,0 +1,183 @@ +#!/usr/bin/perl +# Config writers must use current line numbers and retain nested locks. + +use strict; +use warnings; +use Test::More; +use File::Basename qw(dirname); +use File::Path qw(make_path); +use File::Spec; +use File::Temp qw(tempdir); +use Cwd qw(abs_path); +use POSIX (); + +my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..', '..')); +my $tmp = abs_path(tempdir(CLEANUP => 1)); +my $conf = "$tmp/nginx.conf"; +my $included = "$tmp/servers.conf"; +make_path("$tmp/config/nginx", "$tmp/var"); + +sub write_text +{ +my ($file, $text) = @_; +open(my $fh, '>', $file) or die "$file: $!"; +print $fh $text; +close($fh) or die "$file: $!"; +} + +write_text("$tmp/config/config", "os_type=unix\nos_version=1\n"); +write_text("$tmp/config/miniserv.conf", "root=$root\n"); +write_text("$tmp/config/nginx/config", + "nginx_config=$conf\nnginx_cmd=/bin/true\n"); +write_text($conf, "http {\n}\n"); +$ENV{'WEBMIN_CONFIG'} = "$tmp/config"; +$ENV{'WEBMIN_VAR'} = "$tmp/var"; +$ENV{'FOREIGN_MODULE_NAME'} = 'nginx'; +$ENV{'FOREIGN_ROOT_DIRECTORY'} = $root; +$ENV{'REMOTE_USER'} = 'root'; +unshift(@INC, $root); +require "$root/nginx/nginx-lib.pl"; +{ no warnings 'once'; $main::error_must_die = 1; } + +sub servers +{ +return join('', map { + " server {\n server_name $_;\n listen 80;\n }\n" +} qw(alpha.invalid beta.invalid)); +} + +sub server +{ +my ($name) = @_; +my $http = main::find('http', main::get_config()); +my ($server) = grep { main::find_value('server_name', $_) eq $name } + main::find('server', $http); +die "Missing server $name\n" if (!$server); +return $server; +} + +sub add_ssl +{ +my ($name) = @_; +my $server = server($name); +main::save_directive($server, 'listen', + [{ words => [80] }, { words => [443, 'ssl'] }]); +main::save_directive($server, 'ssl_certificate', ["/$name.pem"]); +main::flush_config_file_lines(); +} + +foreach my $case ([0, 0], [0, 1], [1, 0], [1, 1]) { + my ($split, $cached_lines) = @$case; + my $name = ($split ? 'included server file' : 'single config file'). + ($cached_lines ? ' with cached lines' : ' without cached lines'); + subtest $name => sub { + write_text($conf, "http {\n". + ($split ? " include $included;\n" : servers())."}\n"); + write_text($included, servers()) if ($split); + main::flush_config_cache(); + my $file = $split ? $included : $conf; + main::unflush_file_lines($file); + + # A reader caches the old config. A separate writer then inserts + # lines into the first server before the reader edits the second. + main::get_config(); + main::read_file_lines($file, 1) if $cached_lines; + my $pid = fork(); + die "fork: $!" if (!defined($pid)); + if (!$pid) { + main::lock_all_config_files(); + add_ssl('alpha.invalid'); + main::unlock_all_config_files(); + POSIX::_exit(0); + } + waitpid($pid, 0); + is($?, 0, 'other writer completed'); + main::lock_all_config_files(); + add_ssl('beta.invalid'); + main::unlock_all_config_files(); + main::flush_config_cache(); + foreach my $name (qw(alpha.invalid beta.invalid)) { + my $s = server($name); + is_deeply([map { $_->{'words'} } main::find('listen', $s)], + [[80], [443, 'ssl']], "$name retains both listeners once"); + is(main::find_value('ssl_certificate', $s), + "/$name.pem", "$name retains its own certificate"); + } + + # A nested certificate update must neither discard pending writes + # nor release the lock protecting the outer operation. + main::lock_all_config_files(); + my $s = server('beta.invalid'); + main::save_directive($s, 'ssl_certificate_key', ['/beta.key']); + main::lock_all_config_files($s); + is(server('beta.invalid'), $s, 'nested lock keeps object identity'); + main::unlock_all_config_files(); + ok(-e "$conf.lock", 'nested unlock retains main config lock'); + ok(-e "$file.lock", 'nested unlock retains server file lock'); + main::flush_config_file_lines(); + main::unlock_all_config_files(); + ok(!-e "$conf.lock" && !-e "$file.lock", 'outer unlock releases locks'); + main::flush_config_cache(); + is(main::find_value('ssl_certificate_key', server('beta.invalid')), + '/beta.key', 'nested operation preserves pending edit'); + }; +} + +subtest 'new includes are discovered under the main lock' => sub { + write_text($conf, "http {\n}\n"); + main::flush_config_cache(); + main::get_config(); + write_text($conf, "http {\n include $included;\n}\n"); + main::lock_all_config_files(); + ok(-e "$included.lock", 'newly included file is locked'); + ok(server('alpha.invalid'), 'newly included server is visible'); + main::unlock_all_config_files(); +}; + +subtest 'locks acquired by a caller remain owned by it' => sub { + main::lock_file($conf); + main::lock_all_config_files(); + main::unlock_all_config_files(); + ok(-e "$conf.lock", 'pre-existing lock is retained'); + main::unlock_file($conf); +}; + +subtest 'included files are refreshed after waiting for their locks' => sub { + my $extra = "$tmp/extra.conf"; + write_text($extra, "server {\n server_name gamma.invalid;\n}\n"); + my $lock = \&main::lock_file; + my $changed = 0; + { + no warnings 'redefine'; + local *main::lock_file = sub { + # Model a direct file editor finishing after our include scan. + if ($_[0] eq $included && !$changed++) { + write_text($included, servers()."include $extra;\n"); + } + return $lock->(@_); + }; + main::lock_all_config_files(); + } + ok(-e "$extra.lock", 'new include is locked too'); + ok(server('gamma.invalid'), 'updated include is parsed after locking'); + main::unlock_all_config_files(); +}; + +subtest 'lock acquisition failure releases locks already acquired' => sub { + my $lock = \&main::lock_file; + { + no warnings 'redefine'; + local *main::lock_file = sub { + return 0 if $_[0] eq $included; + return $lock->(@_); + }; + eval { main::lock_all_config_files(); }; + like($@, qr/Failed to lock Nginx config file/, 'failed lock aborts the edit'); + } + ok(!-e "$conf.lock", 'main lock is released after failure'); + main::lock_all_config_files(); + ok(-e "$conf.lock", 'another acquisition works after failure'); + main::unlock_all_config_files(); +}; + +done_testing(); From 0c7fc411aa818c9b52c029df8f29d3a7fa125bcf Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Tue, 15 Sep 2026 01:37:46 +0200 Subject: [PATCH 2/7] Fix to change Nginx lock check to use proper API https://github.com/webmin/webmin/pull/2842#discussion_r4010494535 --- nginx/nginx-lib.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index a1afe3370..9f018a43e 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -554,10 +554,14 @@ my $ok = eval { my %locked; while (@files) { foreach my $f (@files) { - if (&lock_file($f)) { + my $pid = &test_lock($f); + if ($pid && $pid == $$) { + # Keep a lock taken by the caller outside our unlock list. + } + elsif (&lock_file($f)) { push(@lock_all_config_files_cache, $f); } - elsif (!defined($main::locked_file_list{&translate_filename($f)})) { + else { &error("Failed to lock Nginx config file $f"); } $locked{$f} = 1; From 9766789a94521bab7a4a810027594a3a0c4f4927 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Tue, 15 Sep 2026 05:17:58 +0200 Subject: [PATCH 3/7] Drop redundant Nginx lock error cleanup --- nginx/nginx-lib.pl | 50 +++++++++++++++++------------------------- nginx/t/config-locks.t | 17 -------------- 2 files changed, 20 insertions(+), 47 deletions(-) diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index 9f018a43e..7487b4179 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -549,38 +549,28 @@ if ($lock_all_config_files_depth) { # Lock the main file before parsing, so another writer cannot change the # include list or directive line numbers while we acquire the remaining locks. my $main = &resolve_links($config{'nginx_config'}) || $config{'nginx_config'}; -my $ok = eval { - my @files = ($main); - my %locked; - while (@files) { - foreach my $f (@files) { - my $pid = &test_lock($f); - if ($pid && $pid == $$) { - # Keep a lock taken by the caller outside our unlock list. - } - elsif (&lock_file($f)) { - push(@lock_all_config_files_cache, $f); - } - else { - &error("Failed to lock Nginx config file $f"); - } - $locked{$f} = 1; - &unflush_file_lines($f); +my @files = ($main); +my %locked; +while (@files) { + foreach my $f (@files) { + my $pid = &test_lock($f); + if ($pid && $pid == $$) { + # Keep a lock taken by the caller outside our unlock list. } - # An included file may also be edited directly. Re-read after - # waiting for its lock and pick up any newly included files. - &flush_config_cache(); - @files = grep { !$locked{$_} } &unique(&get_all_config_files(), - $parent ? &get_all_config_files($parent) : ()); + elsif (&lock_file($f)) { + push(@lock_all_config_files_cache, $f); + } + else { + &error("Failed to lock Nginx config file $f"); + } + $locked{$f} = 1; + &unflush_file_lines($f); } - 1; - }; -my $err = $@; -if (!$ok) { - # Do not leave partially acquired locks behind if parsing or locking fails. - &unlock_file($_) foreach reverse(@lock_all_config_files_cache); - @lock_all_config_files_cache = (); - die $err; + # An included file may also be edited directly. Re-read after + # waiting for its lock and pick up any newly included files. + &flush_config_cache(); + @files = grep { !$locked{$_} } &unique(&get_all_config_files(), + $parent ? &get_all_config_files($parent) : ()); } $lock_all_config_files_depth = 1; } diff --git a/nginx/t/config-locks.t b/nginx/t/config-locks.t index f27e49e33..3fa9110d6 100644 --- a/nginx/t/config-locks.t +++ b/nginx/t/config-locks.t @@ -163,21 +163,4 @@ subtest 'included files are refreshed after waiting for their locks' => sub { main::unlock_all_config_files(); }; -subtest 'lock acquisition failure releases locks already acquired' => sub { - my $lock = \&main::lock_file; - { - no warnings 'redefine'; - local *main::lock_file = sub { - return 0 if $_[0] eq $included; - return $lock->(@_); - }; - eval { main::lock_all_config_files(); }; - like($@, qr/Failed to lock Nginx config file/, 'failed lock aborts the edit'); - } - ok(!-e "$conf.lock", 'main lock is released after failure'); - main::lock_all_config_files(); - ok(-e "$conf.lock", 'another acquisition works after failure'); - main::unlock_all_config_files(); -}; - done_testing(); From 1c80e8f298faec2e4cdd2a215de48f8914a69736 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Thu, 17 Sep 2026 03:24:37 +0200 Subject: [PATCH 4/7] Update Nginx lock loop comment --- nginx/nginx-lib.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index 7487b4179..fd6055628 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -566,8 +566,8 @@ while (@files) { $locked{$f} = 1; &unflush_file_lines($f); } - # An included file may also be edited directly. Re-read after - # waiting for its lock and pick up any newly included files. + # Raw config edits lock included files independently. Re-read after + # acquiring each batch and lock any newly discovered includes. &flush_config_cache(); @files = grep { !$locked{$_} } &unique(&get_all_config_files(), $parent ? &get_all_config_files($parent) : ()); From afb0929dd9f09d2bbd63d5388282049b0d212ad0 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Thu, 17 Sep 2026 20:14:42 +0200 Subject: [PATCH 5/7] Update Nginx lock discovery loop https://github.com/webmin/webmin/pull/2842#discussion_r4039699490 --- nginx/nginx-lib.pl | 14 ++++++++++---- nginx/t/config-locks.t | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index fd6055628..c36dbb80e 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -551,8 +551,11 @@ if ($lock_all_config_files_depth) { my $main = &resolve_links($config{'nginx_config'}) || $config{'nginx_config'}; my @files = ($main); my %locked; -while (@files) { +# Repeat until a fresh parse finds no additional config files. +while (1) { + my $new_files = 0; foreach my $f (@files) { + next if ($locked{$f}); my $pid = &test_lock($f); if ($pid && $pid == $$) { # Keep a lock taken by the caller outside our unlock list. @@ -564,12 +567,15 @@ while (@files) { &error("Failed to lock Nginx config file $f"); } $locked{$f} = 1; + $new_files++; &unflush_file_lines($f); } - # Raw config edits lock included files independently. Re-read after - # acquiring each batch and lock any newly discovered includes. + last if (!$new_files); + + # An included file may change while this process waits for its lock. + # Reparse after each batch to find any newly included files. &flush_config_cache(); - @files = grep { !$locked{$_} } &unique(&get_all_config_files(), + @files = &unique(&get_all_config_files(), $parent ? &get_all_config_files($parent) : ()); } $lock_all_config_files_depth = 1; diff --git a/nginx/t/config-locks.t b/nginx/t/config-locks.t index 3fa9110d6..8f8fbb069 100644 --- a/nginx/t/config-locks.t +++ b/nginx/t/config-locks.t @@ -163,4 +163,23 @@ subtest 'included files are refreshed after waiting for their locks' => sub { main::unlock_all_config_files(); }; +subtest 'a parent block can span included files' => sub { + write_text($included, "listen 80;\n"); + write_text($conf, + "http {\n". + " server {\n". + " server_name parent.invalid;\n". + " include $included;\n". + " }\n". + "}\n"); + main::flush_config_cache(); + my $parent = server('parent.invalid'); + is_deeply([sort(main::get_all_config_files($parent))], + [sort($conf, $included)], 'parent spans both config files'); + main::lock_all_config_files($parent); + ok(-e "$conf.lock" && -e "$included.lock", + 'parent lock covers both config files'); + main::unlock_all_config_files(); +}; + done_testing(); From a8a1b700150f60539f2c9e65992ef5e98e86f7ae Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Fri, 18 Sep 2026 18:13:47 +0200 Subject: [PATCH 6/7] Fix Nginx cache handling for caller-owned locks --- nginx/nginx-lib.pl | 3 ++- nginx/t/config-locks.t | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index c36dbb80e..a00dae968 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -562,13 +562,14 @@ while (1) { } elsif (&lock_file($f)) { push(@lock_all_config_files_cache, $f); + # Drop stale lines only after acquiring this lock ourselves. + &unflush_file_lines($f); } else { &error("Failed to lock Nginx config file $f"); } $locked{$f} = 1; $new_files++; - &unflush_file_lines($f); } last if (!$new_files); diff --git a/nginx/t/config-locks.t b/nginx/t/config-locks.t index 8f8fbb069..f68fff3ed 100644 --- a/nginx/t/config-locks.t +++ b/nginx/t/config-locks.t @@ -142,8 +142,32 @@ subtest 'locks acquired by a caller remain owned by it' => sub { main::unlock_file($conf); }; +subtest 'pending edits under a caller lock are retained' => sub { + write_text($conf, "http {\n}\n"); + main::flush_config_cache(); + main::unflush_file_lines($conf); + main::lock_file($conf); + my $lines = main::read_file_lines($conf); + splice(@$lines, 1, 0, ' # pending edit'); + main::lock_all_config_files(); + is(main::read_file_lines($conf), $lines, + 'pre-existing writable cache is retained'); + main::flush_file_lines($conf); + main::unlock_all_config_files(); + main::unlock_file($conf); + open(my $fh, '<', $conf) or die "$conf: $!"; + my $saved = do { local $/; <$fh> }; + close($fh) or die "$conf: $!"; + like($saved, qr/^ # pending edit$/m, 'pending edit is saved'); +}; + subtest 'included files are refreshed after waiting for their locks' => sub { my $extra = "$tmp/extra.conf"; + write_text($conf, "http {\n include $included;\n}\n"); + write_text($included, servers()); + main::flush_config_cache(); + main::unflush_file_lines($conf); + main::unflush_file_lines($included); write_text($extra, "server {\n server_name gamma.invalid;\n}\n"); my $lock = \&main::lock_file; my $changed = 0; From c134426e2ae317f70cfaaacbde1bbc9ba9b626ed Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Sat, 19 Sep 2026 01:10:42 +0200 Subject: [PATCH 7/7] Drop unused Nginx lock parent arg --- nginx/nginx-lib.pl | 9 +++------ nginx/t/config-locks.t | 21 +-------------------- 2 files changed, 4 insertions(+), 26 deletions(-) diff --git a/nginx/nginx-lib.pl b/nginx/nginx-lib.pl index a00dae968..aa8896c71 100644 --- a/nginx/nginx-lib.pl +++ b/nginx/nginx-lib.pl @@ -534,12 +534,11 @@ foreach my $f (@files) { @open_config_files = ( ); } -# lock_all_config_files([&parent]) +# lock_all_config_files() # Locks all config files and refreshes the config on the outermost call. # Fetch directive objects after locking; earlier objects may have stale lines. sub lock_all_config_files { -my ($parent) = @_; if ($lock_all_config_files_depth) { # Nested edits share the caller's config tree and pending changes. $lock_all_config_files_depth++; @@ -576,17 +575,15 @@ while (1) { # An included file may change while this process waits for its lock. # Reparse after each batch to find any newly included files. &flush_config_cache(); - @files = &unique(&get_all_config_files(), - $parent ? &get_all_config_files($parent) : ()); + @files = &get_all_config_files(); } $lock_all_config_files_depth = 1; } -# unlock_all_config_files([&parent]) +# unlock_all_config_files() # Un-locks all files used in the current config sub unlock_all_config_files { -my ($parent) = @_; return if (!$lock_all_config_files_depth); return if (--$lock_all_config_files_depth); foreach my $f (reverse(@lock_all_config_files_cache)) { diff --git a/nginx/t/config-locks.t b/nginx/t/config-locks.t index f68fff3ed..89d38190d 100644 --- a/nginx/t/config-locks.t +++ b/nginx/t/config-locks.t @@ -109,7 +109,7 @@ foreach my $case ([0, 0], [0, 1], [1, 0], [1, 1]) { main::lock_all_config_files(); my $s = server('beta.invalid'); main::save_directive($s, 'ssl_certificate_key', ['/beta.key']); - main::lock_all_config_files($s); + main::lock_all_config_files(); is(server('beta.invalid'), $s, 'nested lock keeps object identity'); main::unlock_all_config_files(); ok(-e "$conf.lock", 'nested unlock retains main config lock'); @@ -187,23 +187,4 @@ subtest 'included files are refreshed after waiting for their locks' => sub { main::unlock_all_config_files(); }; -subtest 'a parent block can span included files' => sub { - write_text($included, "listen 80;\n"); - write_text($conf, - "http {\n". - " server {\n". - " server_name parent.invalid;\n". - " include $included;\n". - " }\n". - "}\n"); - main::flush_config_cache(); - my $parent = server('parent.invalid'); - is_deeply([sort(main::get_all_config_files($parent))], - [sort($conf, $included)], 'parent spans both config files'); - main::lock_all_config_files($parent); - ok(-e "$conf.lock" && -e "$included.lock", - 'parent lock covers both config files'); - main::unlock_all_config_files(); -}; - done_testing();