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();