From 466f499e65a9f2349c6cc3f4927b54b2f21f87fa Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Mon, 3 Aug 2026 02:36:05 +0200 Subject: [PATCH 1/4] Fix openSUSE logrotate vendor overlay handling This PR updates the Log File Rotation module for openSUSE Leap 16's split `/usr/etc` and `/etc` configuration model. - Select the local main configuration when present, otherwise use the vendor configuration. - Recursively merge vendor and local drop-ins using the same precedence as `logrotate-all`. - Display vendor configurations while keeping `/usr/etc` read-only. - Copy vendor files into matching `/etc` overrides before editing or deleting them. - Preserve empty overrides so deleted vendor rotations remain disabled. - Use the openSUSE wrapper for scheduled rotation and the effective configuration set for forced rotation. - Keep behavior unchanged on distributions that do not enable the vendor overlay. Includes regression coverage for selection, precedence, copy-on-write, deletion, scheduling, backups, and protection against direct vendor writes. Closes [#2682](https://github.com/webmin/webmin/issues/2682). --- logrotate/backup_config.pl | 14 +- logrotate/config-openSUSE-Linux-16.0-ALL | 3 + logrotate/delete_logs.cgi | 13 ++ logrotate/edit_log.cgi | 17 ++ logrotate/edit_sched.cgi | 8 +- logrotate/force.cgi | 8 +- logrotate/index.cgi | 19 +- logrotate/install_check.pl | 4 +- logrotate/lang/en | 8 + logrotate/logrotate-lib.pl | 246 ++++++++++++++++++++++- logrotate/save_log.cgi | 14 ++ logrotate/save_sched.cgi | 5 +- logrotate/t/run-tests.t | 220 +++++++++++++++++--- 13 files changed, 527 insertions(+), 52 deletions(-) diff --git a/logrotate/backup_config.pl b/logrotate/backup_config.pl index 9a8c1be45..d25f2f837 100755 --- a/logrotate/backup_config.pl +++ b/logrotate/backup_config.pl @@ -5,8 +5,18 @@ do 'logrotate-lib.pl'; # Returns files and directories that can be backed up sub backup_config_files { -local $conf = &get_config(); -return &unique(map { $_->{'file'} } @$conf); +# Keep backup behavior unchanged on systems without the vendor overlay. +if (!$config{'vendor_logrotate_conf'} && !$config{'vendor_add_file'}) { + local $conf = &get_config(); + return &unique(map { $_->{'file'} } @$conf); + } + +# Back up only writable files. Use the complete effective file list so an +# empty local file that intentionally shadows a vendor file is preserved. +local ($conf, $lnum, $files) = &get_config(); +return &unique(grep { !&is_vendor_main_config($_) && + !&is_vendor_config_file($_) } + @$files); } # pre_backup(&files) diff --git a/logrotate/config-openSUSE-Linux-16.0-ALL b/logrotate/config-openSUSE-Linux-16.0-ALL index 6ddca8e5c..af724fb7f 100644 --- a/logrotate/config-openSUSE-Linux-16.0-ALL +++ b/logrotate/config-openSUSE-Linux-16.0-ALL @@ -1,5 +1,8 @@ sort_mode=0 logrotate_conf=/etc/logrotate.conf +vendor_logrotate_conf=/usr/etc/logrotate.conf add_file=/etc/logrotate.d +vendor_add_file=/usr/etc/logrotate.d scan_add_file=1 logrotate=logrotate +logrotate_all=/usr/sbin/logrotate-all diff --git a/logrotate/delete_logs.cgi b/logrotate/delete_logs.cgi index ea0af75f2..bf6477aaf 100755 --- a/logrotate/delete_logs.cgi +++ b/logrotate/delete_logs.cgi @@ -12,6 +12,19 @@ require './logrotate-lib.pl'; # Delete the sections $parent = &get_config_parent(); $conf = $parent->{'members'}; + +# Copy each selected vendor file to the local override tree before changing +# it. Reload the parsed configuration after copying so all line references +# point at the writable files. +%vendor_files = map { $conf->[$_]->{'file'}, 1 } + grep { &is_vendor_config_file($conf->[$_]->{'file'}) } @d; +if (%vendor_files) { + foreach $f (keys %vendor_files) { + &ensure_local_config_override($f); + } + $parent = &get_config_parent(); + $conf = $parent->{'members'}; + } foreach $d (sort { $b <=> $a } @d) { $log = $conf->[$d]; &lock_file($log->{'file'}); diff --git a/logrotate/edit_log.cgi b/logrotate/edit_log.cgi index 5124ec254..6ef61531d 100755 --- a/logrotate/edit_log.cgi +++ b/logrotate/edit_log.cgi @@ -20,6 +20,23 @@ else { $lconf = $log->{'members'}; } +# Explain which side of the vendor/local overlay is displayed and where a +# copy-on-write edit will be saved before presenting the form. +if ($in{'global'} && &is_vendor_main_config(&get_main_config_file())) { + print &ui_alert_box(&text('global_vendor', + "".&html_escape($config{'logrotate_conf'}).""), + 'info'); + } +elsif ($log && &is_vendor_config_file($log->{'file'})) { + print &ui_alert_box(&text('edit_vendor', + "".&html_escape(&get_local_override_file( + $log->{'file'})).""), 'info'); + } +elsif ($log && (my $vendor = &get_vendor_config_file($log->{'file'}))) { + print &ui_alert_box(&text('edit_override', + "".&html_escape($vendor).""), 'info'); + } + print &ui_form_start("save_log.cgi", "post", undef, "id='edit_log_form'"); print &ui_hidden("new", $in{'new'}),"\n"; print &ui_hidden("idx", $in{'idx'}),"\n"; diff --git a/logrotate/edit_sched.cgi b/logrotate/edit_sched.cgi index 97bfb8d02..61d64d9dd 100755 --- a/logrotate/edit_sched.cgi +++ b/logrotate/edit_sched.cgi @@ -5,7 +5,13 @@ require './logrotate-lib.pl'; &ui_print_header(undef, $text{'sched_title'}, ""); -print "

",&text('sched_desc', "$config{'logrotate'}"),"

\n"; +# Show the wrapper or fallback command on vendor-overlay systems, while +# retaining the original short program name everywhere else. +my $sched_command = ($config{'logrotate_all'} || + $config{'vendor_logrotate_conf'} || $config{'vendor_add_file'}) ? + &get_scheduled_logrotate_command() : $config{'logrotate'}; +print "

",&text('sched_desc', "". + &html_escape($sched_command).""),"

\n"; # Find the job, looking in daily directories too &foreign_require("cron", "cron-lib.pl"); diff --git a/logrotate/force.cgi b/logrotate/force.cgi index 0a8bbf2d1..c1fde6870 100755 --- a/logrotate/force.cgi +++ b/logrotate/force.cgi @@ -10,8 +10,12 @@ $SIG{'TERM'} = 'IGNORE'; print $text{'force_doing'},"\n"; &clean_environment(); -my (undef, undef, $files) = &get_config($config{'logrotate_conf'}); -my @configs = ($config{'logrotate_conf'}, &get_add_file_configs($files)); + +# Force the same effective main and drop-in configs selected by the distro +# wrapper, while avoiding duplicate files already reached through includes. +my $main = &get_main_config_file(); +my (undef, undef, $files) = &get_config($main); +my @configs = ($main, &get_add_file_configs($files)); my $configs = join(" ", map { "e_path($_) } @configs); $out = &backquote_logged("$config{'logrotate'} -f $configs 2>&1"); &reset_environment(); diff --git a/logrotate/index.cgi b/logrotate/index.cgi index 7ee559eb6..802205c62 100755 --- a/logrotate/index.cgi +++ b/logrotate/index.cgi @@ -19,11 +19,12 @@ if (!&has_command($config{'logrotate'})) { &ui_print_footer("/", $text{'index'}); exit; } -if (!-r $config{'logrotate_conf'}) { +my $main_config = &get_main_config_file(); +if (!-r $main_config) { &ui_print_header(undef, $text{'index_title'}, "", "intro", 1, 1); &ui_print_endpage( &ui_config_link('index_econf', - [ "$config{'logrotate_conf'}", undef ])); + [ "$main_config", undef ])); } # Get the version @@ -52,9 +53,19 @@ foreach $c ($config{'sort_mode'} ? local $p = &get_period($c->{'members'}) || $defp; local $r = &find_value("postrotate", $c->{'members'}); $r =~ s/\n/
\n/g; + local $label = join(" ", map { "$_
" } + @{$c->{'name'}}); + + # Distinguish read-only vendor entries from writable local files + # that shadow a vendor entry at the same relative path. + if (&is_vendor_config_file($c->{'file'})) { + $label .= "$text{'index_vendor'}"; + } + elsif (&get_vendor_config_file($c->{'file'})) { + $label .= "$text{'index_override'}"; + } push(@table, [ &ui_link("edit_log.cgi?idx=".$c->{'index'}, - join(" ", map { "$_
" } - @{$c->{'name'}}) ), + $label), $text{'period_'.$p} || "$text{'index_notset'}", $r ? "$r" diff --git a/logrotate/install_check.pl b/logrotate/install_check.pl index 60a66a796..0d5ca01c0 100755 --- a/logrotate/install_check.pl +++ b/logrotate/install_check.pl @@ -6,7 +6,9 @@ do 'logrotate-lib.pl'; # For mode 0, returns 1 if installed, 0 if not. sub is_installed { -return 0 if (!-r $config{'logrotate_conf'} && !-r $config{'sample_conf'}); +# Accept the vendor main config when the optional local override is absent. +my $main = &get_main_config_file(); +return 0 if (!-r $main && !-r $config{'sample_conf'}); return 0 if (!&has_command($config{'logrotate'})); return $_[0] ? 2 : 1; } diff --git a/logrotate/lang/en b/logrotate/lang/en index 8d57c99be..9cc0c7d95 100644 --- a/logrotate/lang/en +++ b/logrotate/lang/en @@ -19,6 +19,8 @@ index_force=Force Log Rotation index_forcedesc=Force the immediate rotation of all log files, even if it is not yet time. index_logrotate=Logrotate index_delete=Delete Selected Log Rotations +index_vendor=Vendor configuration +index_override=Local vendor override period_daily=Daily period_weekly=Weekly @@ -65,11 +67,17 @@ edit_default=Default ($1) edit_sharedscripts=Only run scripts once for all files? edit_now=Rotate Now edit_clone=Clone +edit_vendor=This is a vendor-provided configuration. Saving or deleting it will first create the local override $1; the file under /usr/etc will not be changed. +edit_override=This local configuration overrides the vendor file $1. global_title=Global Options global_header=Default options for all log files +global_vendor=These defaults currently come from the vendor configuration. Saving will first create the writable local configuration $1; the file under /usr/etc will not be changed. save_err=Failed to save log +save_eoverride=Cannot create local override $1 because that path already exists and is not a regular file. +save_ecopy=Failed to create local override $1: $2 +save_evendorwrite=Refusing to modify vendor configuration $1 without first creating a local override. save_efile=Missing or invalid log filename save_esize=Missing or invalid maximum size save_eminsize=Missing or invalid minimum size diff --git a/logrotate/logrotate-lib.pl b/logrotate/logrotate-lib.pl index 38a09fa23..f1a2d1c60 100755 --- a/logrotate/logrotate-lib.pl +++ b/logrotate/logrotate-lib.pl @@ -15,10 +15,15 @@ if (!-r $config{'logrotate_conf'} && -r $config{'sample_conf'}) { ©_source_dest($config{'sample_conf'}, $config{'logrotate_conf'}); } +# get_config_parent() +# Returns the parsed global config while keeping the writable local file as +# its save target. Callers must materialize that file before global writes. sub get_config_parent { if (!$get_config_parent_cache) { local ($conf, $lines) = &get_config(); + # Even when members came from the vendor config, never make /usr the + # destination for newly-added global directives. $get_config_parent_cache = { 'members' => $conf, 'file' => $config{'logrotate_conf'}, 'line' => 0, @@ -28,28 +33,228 @@ if (!$get_config_parent_cache) { return $get_config_parent_cache; } +# get_main_config_file() +# Returns the local main config, or the vendor default if no local one exists +sub get_main_config_file +{ +return $config{'logrotate_conf'} if (-e $config{'logrotate_conf'}); +return $config{'vendor_logrotate_conf'} + if ($config{'vendor_logrotate_conf'}); +return $config{'logrotate_conf'}; +} + +# is_vendor_main_config(file) +# Returns 1 if a file is the vendor-provided main config +sub is_vendor_main_config +{ +my ($file) = @_; +return $config{'vendor_logrotate_conf'} && + &same_file($file, $config{'vendor_logrotate_conf'}); +} + +# relative_config_path(file, directory) +# Returns a file's path relative to a config directory +sub relative_config_path +{ +my ($file, $dir) = @_; +return undef if (!$file || !$dir); +$dir =~ s/\/+$//; +$dir .= '/'; +return $file =~ /^\Q$dir\E(.+)$/ ? $1 : undef; +} + +# is_vendor_config_file(file) +# Returns 1 if a drop-in comes from the vendor directory +sub is_vendor_config_file +{ +my ($file) = @_; +return defined(&relative_config_path( + $file, $config{'vendor_add_file'})); +} + +# get_local_override_file(vendor-file) +# Returns the local path that overrides a vendor drop-in +sub get_local_override_file +{ +my ($file) = @_; +my $rel = &relative_config_path($file, $config{'vendor_add_file'}); +return undef if (!defined($rel) || !$config{'add_file'}); +return $config{'add_file'}.'/'.$rel; +} + +# get_vendor_config_file(local-file) +# Returns the vendor file shadowed by a local drop-in, if any +sub get_vendor_config_file +{ +my ($file) = @_; +my $rel = &relative_config_path($file, $config{'add_file'}); +return undef if (!defined($rel) || !$config{'vendor_add_file'}); +my $vendor = $config{'vendor_add_file'}.'/'.$rel; +return -f $vendor ? $vendor : undef; +} + +# flush_logrotate_config_cache() +# Clears parsed config state after creating a local override +sub flush_logrotate_config_cache +{ +%get_config_cache = ( ); +%get_config_lnum_cache = ( ); +%get_config_files_cache = ( ); +$get_config_parent_cache = undef; +} + +# copy_vendor_config(source, destination) +# Copies a vendor config to the writable local tree +sub copy_vendor_config +{ +my ($source, $dest) = @_; + +# An existing regular destination is already a usable override. Refuse a +# destination symlink or other file type so it cannot redirect this write. +if (-e $dest || -l $dest) { + if (-f $dest && !-l $dest) { + &flush_logrotate_config_cache(); + return $dest; + } + &error(&text('save_eoverride', "". + &html_escape($dest)."")); + } + +# Create missing subdirectories before copying the complete vendor file. +# Following a source symlink produces an editable snapshot, not another link. +my $dir = $dest; +$dir =~ s/\/[^\/]+$//; +&make_dir_recursive($dir, 0755) if (!-d $dir); +my ($ok, $err) = ©_source_dest($source, $dest, 1); + +# Do not leave a partial override behind after a copy or chmod failure, since +# even an incomplete local file would hide the valid vendor configuration. +if (!$ok || !&set_ownership_permissions(undef, undef, 0644, $dest)) { + $err ||= $!; + &unlink_file($dest) if (-e $dest || -l $dest); + &error(&text('save_ecopy', "".&html_escape($dest)."", + &html_escape($err))); + } + +# Force the next read to select and parse the newly-created local file. +&flush_logrotate_config_cache(); +return $dest; +} + +# ensure_local_main_config() +# Creates a writable local main config when only the vendor default exists +sub ensure_local_main_config +{ +my $main = &get_main_config_file(); +return $config{'logrotate_conf'} + if (!&is_vendor_main_config($main)); +return ©_vendor_config($main, $config{'logrotate_conf'}); +} + +# ensure_local_config_override(vendor-file) +# Creates a writable local copy that shadows a vendor drop-in +sub ensure_local_config_override +{ +my ($file) = @_; +my $local = &get_local_override_file($file); +return $file if (!$local); +return ©_vendor_config($file, $local); +} + +# list_config_dir_files(directory, [relative-subdirectory]) +# Returns relative and absolute paths for regular files below a directory +sub list_config_dir_files +{ +my ($dir, $subdir) = @_; +my $path = $subdir ? $dir.'/'.$subdir : $dir; +opendir(my $dh, $path) || return ( ); +my @names = sort { $a cmp $b } readdir($dh); +closedir($dh); +my @rv; +foreach my $name (@names) { + next if ($name eq '.' || $name eq '..'); + my $rel = $subdir ? $subdir.'/'.$name : $name; + my $file = $dir.'/'.$rel; + + # Match find without -L: ignore symlinks, recurse into real directories, + # and return only regular files with paths relative to the scanned root. + next if (-l $file); + if (-d $file) { + push(@rv, &list_config_dir_files($dir, $rel)); + } + elsif (-f $file) { + push(@rv, [ $rel, $file ]); + } + } +return @rv; +} + # get_add_file_configs([&already-loaded-files]) -# Returns configs loaded externally from the add-file directory +# Returns the effective vendor and local configs loaded by logrotate-all sub get_add_file_configs { my ($files) = @_; -return ( ) if (!$config{'scan_add_file'} || !$config{'add_file'} || - !-d $config{'add_file'}); +return ( ) if (!$config{'scan_add_file'}); + +# Collect the same relative names produced by the wrapper's recursive find. +# Processing the local tree last records its regular files directly. +my %effective; +foreach my $dir ($config{'vendor_add_file'}, $config{'add_file'}) { + next if (!$dir || !-d $dir); + foreach my $entry (&list_config_dir_files($dir)) { + $effective{$entry->[0]} = $entry->[1]; + } + } + +# Match the wrapper's stable lexical order and omit files already reached by +# an explicit include in the main configuration. The existence check also +# honors a local non-regular counterpart exactly as the wrapper does. my @rv; -foreach my $f (glob("$config{'add_file'}/*")) { - next if (!-f $f || $files && +foreach my $name (sort { $a cmp $b } keys %effective) { + my $local = $config{'add_file'} ? + $config{'add_file'}.'/'.$name : undef; + my $f = $local && -e $local ? $local : $effective{$name}; + next if ($files && grep { &same_file($_, $f) } @$files); push(@rv, $f); } return @rv; } +# get_scheduled_logrotate_command() +# Returns the distro wrapper, or a command for the effective config files +sub get_scheduled_logrotate_command +{ +# The distro wrapper discovers the effective drop-in set on every run, so it +# remains correct when packages or administrators add files later. +if ($config{'logrotate_all'} && -x $config{'logrotate_all'}) { + return "e_path($config{'logrotate_all'}); + } + +# Preserve the historical command exactly on systems that do not opt into +# external or vendor configuration discovery. +if (!$config{'vendor_logrotate_conf'} && !$config{'vendor_add_file'} && + !$config{'scan_add_file'}) { + return &has_command($config{'logrotate'})." ". + $config{'logrotate_conf'}; + } + +# If the configured wrapper is unavailable, build a usable command from the +# effective main config and the drop-ins visible at schedule creation time. +my $main = &get_main_config_file(); +my (undef, undef, $files) = &get_config($main); +my @configs = ($main, &get_add_file_configs($files)); +my $program = &has_command($config{'logrotate'}) || $config{'logrotate'}; +return "e_path($program).' '. + join(' ', map { "e_path($_) } @configs); +} + # get_config([file]) # Returns a list of logrotate config file entries sub get_config { my ($argfile) = @_; -my $file = $argfile || $config{'logrotate_conf'}; +my $file = $argfile || &get_main_config_file(); if (!$argfile && $get_config_cache{$file}) { return wantarray ? ( $get_config_cache{$file}, $get_config_lnum_cache{$file}, @@ -230,10 +435,33 @@ sub save_directive my ($parent, $oldv, $newv, $indent) = @_; my $conf = $parent->{'members'}; my $old = !defined($oldv) ? undef : ref($oldv) ? $oldv : &find($oldv, $conf); -my $lref = &read_file_lines($old ? $old->{'file'} : $parent->{'file'}); my $new = !defined($newv) ? undef : ref($newv) ? $newv : { 'name' => $old ? $old->{'name'} : $oldv, 'value' => $newv }; + +# Refuse direct vendor writes even if a caller forgets to materialize the +# local copy first. New log sections may still be written to their explicit +# local file while the global defaults continue to come from the vendor file. +my $vendor_file; +if ($old) { + my $shadowed_vendor = &get_vendor_config_file($old->{'file'}); + if (&is_vendor_main_config($old->{'file'}) || + &is_vendor_config_file($old->{'file'})) { + $vendor_file = $old->{'file'}; + } + elsif ($shadowed_vendor && + &same_file($old->{'file'}, $shadowed_vendor)) { + $vendor_file = $shadowed_vendor; + } + } +elsif (!$old && $new && !$new->{'members'} && $parent->{'global'} && + &is_vendor_main_config(&get_main_config_file())) { + $vendor_file = &get_main_config_file(); + } +&error(&text('save_evendorwrite', + "".&html_escape($vendor_file)."")) if ($vendor_file); + +my $lref = &read_file_lines($old ? $old->{'file'} : $parent->{'file'}); my @lines = &directive_lines($new, $indent) if ($new); my $gparent = &get_config_parent(); if ($old && $new) { @@ -337,10 +565,12 @@ return @rv; } # delete_if_empty(file) -# Remove a file if it has no more lines in the config +# Removes a file if it has no more parsed entries, unless it is a local +# override whose continued existence is needed to hide a vendor file sub delete_if_empty { my ($file) = @_; +return if (&get_vendor_config_file($file)); my $conf = &get_config(); my %files = map { $_, 1 } &unique(map { $_->{'file'} } @$conf); &unlink_file($file) if (!$files{$file}); diff --git a/logrotate/save_log.cgi b/logrotate/save_log.cgi index ad3a194b9..f3691bdee 100755 --- a/logrotate/save_log.cgi +++ b/logrotate/save_log.cgi @@ -4,8 +4,22 @@ require './logrotate-lib.pl'; &ReadParse(); + +# On systems with vendor configuration below /usr, create the writable local +# main config before changing global options. The parent object intentionally +# keeps this local path as its write destination. +&ensure_local_main_config() if ($in{'global'}); $parent = &get_config_parent(); $conf = $parent->{'members'}; + +# A local drop-in shadows the whole vendor file, so copy it intact before +# editing or deleting one section. Rotate Now is read-only and needs no copy. +if (!$in{'global'} && !$in{'new'} && !$in{'now'} && + &is_vendor_config_file($conf->[$in{'idx'}]->{'file'})) { + &ensure_local_config_override($conf->[$in{'idx'}]->{'file'}); + $parent = &get_config_parent(); + $conf = $parent->{'members'}; + } @files = split(/\s+/, $in{'file'}); if ($in{'global'}) { # Editing the global options diff --git a/logrotate/save_sched.cgi b/logrotate/save_sched.cgi index 23a6b027f..c1688eed5 100755 --- a/logrotate/save_sched.cgi +++ b/logrotate/save_sched.cgi @@ -11,9 +11,10 @@ if ($in{'idx'} ne "") { $oldjob = $job = $jobs[$in{'idx'}]; } else { + # Prefer the distro wrapper, when available, so future runs discover the + # then-current vendor and local drop-in set. $job = { 'user' => 'root', - 'command' => &has_command($config{'logrotate'})." ". - $config{'logrotate_conf'}, + 'command' => &get_scheduled_logrotate_command(), 'active' => 1 }; } &lock_file(&cron::cron_file($job)); diff --git a/logrotate/t/run-tests.t b/logrotate/t/run-tests.t index e5efba992..11b0bcc2d 100644 --- a/logrotate/t/run-tests.t +++ b/logrotate/t/run-tests.t @@ -7,15 +7,23 @@ use File::Basename qw(dirname); use File::Path qw(make_path); use File::Temp qw(tempdir); +# Build an isolated openSUSE-style /etc and /usr/etc configuration layout. my $module_dir = abs_path(dirname(abs_path($0))."/.."); my $root_dir = abs_path("$module_dir/.."); my $config_dir = tempdir(CLEANUP => 1); my $var_dir = tempdir(CLEANUP => 1); my $fixture_dir = tempdir(CLEANUP => 1); -my $add_dir = "$fixture_dir/logrotate.d"; -my $main_file = "$fixture_dir/logrotate.conf"; -make_path("$config_dir/logrotate", $add_dir); +my $local_add_dir = "$fixture_dir/etc/logrotate.d"; +my $vendor_add_dir = "$fixture_dir/usr/etc/logrotate.d"; +my $local_main_file = "$fixture_dir/etc/logrotate.conf"; +my $vendor_main_file = "$fixture_dir/usr/etc/logrotate.conf"; +my $wrapper = "$fixture_dir/usr/sbin/logrotate-all"; +make_path("$config_dir/logrotate", $local_add_dir, + "$local_add_dir/nested", "$vendor_add_dir/deep", + "$vendor_add_dir/nested", dirname($wrapper)); +# write_text(file, contents) +# Writes a text fixture and fails the test immediately on an I/O error sub write_text { my ($file, $text) = @_; @@ -24,17 +32,48 @@ print $fh $text; close($fh) or die "close $file: $!"; } +# read_text(file) +# Returns the complete contents of a text fixture +sub read_text +{ +my ($file) = @_; +open(my $fh, "<", $file) or die "open $file: $!"; +local $/; +my $text = <$fh>; +close($fh) or die "close $file: $!"; +return $text; +} + +# Populate both trees with vendor-only, local-only, nested, and overridden +# files so the fixture exercises the wrapper's key overlay rules. +my $vendor_main_text = + "weekly\n/var/log/vendor-main.log {\n\trotate 4\n}\n"; write_text("$config_dir/config", "os_type=linux\nos_version=0\n"); write_text("$config_dir/logrotate/config", "sort_mode=0\n". - "logrotate_conf=$main_file\n". - "add_file=$add_dir\n". + "logrotate_conf=$local_main_file\n". + "vendor_logrotate_conf=$vendor_main_file\n". + "add_file=$local_add_dir\n". + "vendor_add_file=$vendor_add_dir\n". "scan_add_file=1\n". - "logrotate=logrotate\n"); -write_text($main_file, "weekly\n/var/log/main.log {\n\trotate 4\n}\n"); -write_text("$add_dir/one", "/var/log/one.log {\n\tdaily\n}\n"); -write_text("$add_dir/two", "/var/log/two.log {\n\tmonthly\n}\n"); + "logrotate=/bin/echo\n". + "logrotate_all=$wrapper\n"); +write_text($vendor_main_file, $vendor_main_text); +write_text("$vendor_add_dir/one", "/var/log/vendor-one.log {\n\tdaily\n}\n"); +write_text("$vendor_add_dir/shared", + "/var/log/vendor-shared.log {\n\tdaily\n}\n"); +write_text("$vendor_add_dir/deep/vendor", + "/var/log/deep-vendor.log {\n\tmonthly\n}\n"); +write_text("$local_add_dir/local-only", + "/var/log/local-only.log {\n\tweekly\n}\n"); +write_text("$local_add_dir/shared", + "/var/log/local-shared.log {\n\tweekly\n}\n"); +write_text("$local_add_dir/nested/local", + "/var/log/nested-local.log {\n\tweekly\n}\n"); +write_text($wrapper, "#!/bin/sh\nexit 0\n"); +chmod(0755, $wrapper) or die "chmod $wrapper: $!"; +# Point Webmin at the isolated fixture before loading the module library. $ENV{'WEBMIN_CONFIG'} = $config_dir; $ENV{'WEBMIN_VAR'} = $var_dir; $ENV{'FOREIGN_MODULE_NAME'} = 'logrotate'; @@ -42,6 +81,8 @@ $ENV{'FOREIGN_ROOT_DIRECTORY'} = $root_dir; chdir($module_dir) or die "chdir $module_dir: $!"; require "$module_dir/logrotate-lib.pl"; +# clear_config_cache() +# Forces each test phase to parse the configuration from disk again sub clear_config_cache { no warnings 'once'; @@ -51,6 +92,8 @@ no warnings 'once'; $main::get_config_parent_cache = undef; } +# log_names(config) +# Returns only the log path names from parsed rotation sections sub log_names { my ($config) = @_; @@ -58,40 +101,153 @@ return [ map { $_->{'name'}->[0] } grep { $_->{'members'} } @$config ]; } +# The vendor main file is the initial fallback because no local main exists. +is(main::get_main_config_file(), $vendor_main_file, + 'vendor main config is used when no local main config exists'); +ok(main::is_vendor_main_config($vendor_main_file), + 'vendor main config is recognized'); + +# Match the wrapper's existence test rather than requiring a regular file. +my $nonregular_main = "$fixture_dir/etc/nonregular-main"; +make_path($nonregular_main); +{ +local $main::config{'logrotate_conf'} = $nonregular_main; +is(main::get_main_config_file(), $nonregular_main, + 'local main path wins whenever it exists'); +} +{ +local $main::config{'logrotate_conf'} = "$fixture_dir/etc/missing-main"; +local $main::config{'vendor_logrotate_conf'} = + "$fixture_dir/usr/etc/missing-main"; +is(main::get_main_config_file(), $main::config{'vendor_logrotate_conf'}, + 'configured vendor main path is used whenever the local path is absent'); +} + +# The effective list is sorted by relative path, with local files replacing +# vendor files that have the same relative path. +my @effective_add_files = ( + "$vendor_add_dir/deep/vendor", + "$local_add_dir/local-only", + "$local_add_dir/nested/local", + "$vendor_add_dir/one", + "$local_add_dir/shared", + ); my ($config, undef, $files) = main::get_config(); is_deeply(log_names($config), - [ '/var/log/main.log', '/var/log/one.log', '/var/log/two.log' ], - 'opt-in scan loads sections from add_file directory'); + [ '/var/log/vendor-main.log', '/var/log/deep-vendor.log', + '/var/log/local-only.log', '/var/log/nested-local.log', + '/var/log/vendor-one.log', '/var/log/local-shared.log' ], + 'vendor and local trees are recursively merged with local precedence'); is_deeply([ map { $_->{'index'} } grep { $_->{'members'} } @$config ], - [ 1, 2, 3 ], 'scanned sections keep stable top-level indexes'); -is_deeply($files, - [ $main_file, "$add_dir/one", "$add_dir/two" ], - 'file cache contains the primary and scanned configuration files'); -my (undef, undef, $primary_files) = main::get_config($main_file); -is_deeply([ main::get_add_file_configs($primary_files) ], - [ "$add_dir/one", "$add_dir/two" ], - 'force rotation adds externally loaded configuration files'); + [ 1, 2, 3, 4, 5, 6 ], + 'effective sections keep stable top-level indexes'); +is_deeply($files, [ $vendor_main_file, @effective_add_files ], + 'file cache contains the effective main and merged drop-ins'); +ok(!grep({ $_ eq "$vendor_add_dir/shared" } @$files), + 'local file hides the same relative vendor file'); +my (undef, undef, $primary_files) = main::get_config($vendor_main_file); +is_deeply([ main::get_add_file_configs($primary_files) ], + \@effective_add_files, + 'externally loaded configuration files match the effective overlay'); +is(main::get_scheduled_logrotate_command(), main::quote_path($wrapper), + 'scheduled rotations use the distribution wrapper'); + +# Disabling the opt-in must restore the behavior used by other distributions. $main::config{'scan_add_file'} = 0; clear_config_cache(); ($config, undef, $files) = main::get_config(); -is_deeply(log_names($config), [ '/var/log/main.log' ], - 'add_file is not scanned without explicit opt-in'); -is_deeply($files, [ $main_file ], - 'file cache excludes add_file directory when scanning is disabled'); +is_deeply(log_names($config), [ '/var/log/vendor-main.log' ], + 'vendor and local trees are not scanned without explicit opt-in'); +is_deeply($files, [ $vendor_main_file ], + 'file cache excludes external directories when scanning is disabled'); +# The low-level writer must fail closed if a caller skips copy-on-write. +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { + main::save_directive(main::get_config_parent(), 'weekly', ''); + }; +like($@, qr/Refusing to modify vendor configuration/, + 'direct writes to vendor configuration are rejected'); +} + +# Editing global options materializes an exact local copy of the vendor main. $main::config{'scan_add_file'} = 1; -write_text($main_file, - "weekly\ninclude $add_dir\n/var/log/main.log {\n\trotate 4\n}\n"); +clear_config_cache(); +is(main::ensure_local_main_config(), $local_main_file, + 'editing the vendor main config creates a local main config'); +is(read_text($local_main_file), $vendor_main_text, + 'local main config starts as an exact vendor copy'); +is(read_text($vendor_main_file), $vendor_main_text, + 'copying the main config does not alter the vendor file'); +is(main::get_main_config_file(), $local_main_file, + 'local main config takes precedence after it is created'); + +# Editing a nested vendor drop-in copies the whole source file to the same +# relative path in the local tree and immediately switches parser ownership. +my $vendor_dropin = "$vendor_add_dir/deep/vendor"; +my $local_dropin = "$local_add_dir/deep/vendor"; +is(main::ensure_local_config_override($vendor_dropin), $local_dropin, + 'editing a vendor drop-in creates its matching local override'); +is(read_text($local_dropin), read_text($vendor_dropin), + 'local drop-in starts as an exact copy of the whole vendor file'); +is(main::get_local_override_file($vendor_dropin), $local_dropin, + 'vendor drop-in maps to the correct writable path'); +is(main::get_vendor_config_file($local_dropin), $vendor_dropin, + 'local override maps back to the shadowed vendor file'); + +($config, undef, $files) = main::get_config(); +my ($deep_log) = grep { $_->{'members'} && + $_->{'name'}->[0] eq '/var/log/deep-vendor.log' } + @$config; +is($deep_log->{'file'}, $local_dropin, + 'parser switches to the local copy after an override is created'); + +# An empty local file must remain both effective and backup-visible because +# its existence is what prevents the vendor file from becoming active again. +write_text($local_dropin, ''); +clear_config_cache(); +main::delete_if_empty($local_dropin); +ok(-e $local_dropin, + 'empty local override is retained so the vendor file stays disabled'); +(undef, undef, $files) = main::get_config(); +ok(grep({ $_ eq $local_dropin } @$files), + 'empty local override remains in the effective file cache for backups'); +ok(!grep({ $_ eq $vendor_dropin } @$files), + 'empty local override continues to hide the vendor file'); + +# Explicit includes and external discovery must not parse the same file twice. +write_text($local_main_file, + "weekly\ninclude $local_add_dir\n". + "/var/log/main.log {\n\trotate 4\n}\n"); clear_config_cache(); ($config, undef, $files) = main::get_config(); -is_deeply(log_names($config), - [ '/var/log/one.log', '/var/log/two.log', '/var/log/main.log' ], - 'explicitly included files are not loaded a second time'); -is(scalar(grep { main::same_file($_, "$add_dir/one") } @$files), 1, +is(scalar(grep { $_->{'members'} && + $_->{'name'}->[0] eq '/var/log/local-only.log' } + @$config), 1, + 'explicitly included files are not parsed a second time'); +is(scalar(grep { main::same_file($_, "$local_add_dir/local-only") } + @$files), 1, 'explicit include is represented once in the file cache'); -(undef, undef, $primary_files) = main::get_config($main_file); -is_deeply([ main::get_add_file_configs($primary_files) ], [ ], - 'force rotation does not repeat explicitly included files'); + +# A local path selected by the wrapper's existence check wins even when find +# discovers the relative name only from the regular vendor file. +my $edge_dir = tempdir(CLEANUP => 1); +my $edge_local_dir = "$edge_dir/etc/logrotate.d"; +my $edge_vendor_dir = "$edge_dir/usr/etc/logrotate.d"; +my $edge_target = "$edge_dir/local-target"; +make_path($edge_local_dir, $edge_vendor_dir); +write_text("$edge_vendor_dir/linked", "vendor\n"); +write_text($edge_target, "local\n"); +symlink($edge_target, "$edge_local_dir/linked") or + die "symlink $edge_local_dir/linked: $!"; +{ +local $main::config{'add_file'} = $edge_local_dir; +local $main::config{'vendor_add_file'} = $edge_vendor_dir; +is_deeply([ main::get_add_file_configs() ], [ "$edge_local_dir/linked" ], + 'local existing path overrides the matching vendor file'); +} done_testing(); From b7aed037587dde5d06865f17ad8a382c4b6e46f8 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Sun, 9 Aug 2026 18:18:30 +0200 Subject: [PATCH 2/4] Fix to make logrotate vendor overrides work for API callers https://github.com/webmin/webmin/pull/2805#discussion_r3740632921 --- logrotate/logrotate-lib.pl | 116 +++++++++++++++++++++++++++++++------ logrotate/t/run-tests.t | 80 +++++++++++++++++++------ 2 files changed, 159 insertions(+), 37 deletions(-) diff --git a/logrotate/logrotate-lib.pl b/logrotate/logrotate-lib.pl index f1a2d1c60..b809543a8 100755 --- a/logrotate/logrotate-lib.pl +++ b/logrotate/logrotate-lib.pl @@ -109,12 +109,19 @@ sub copy_vendor_config { my ($source, $dest) = @_; -# An existing regular destination is already a usable override. Refuse a -# destination symlink or other file type so it cannot redirect this write. +# An existing independent regular destination is already a usable override. +# Refuse links to the vendor file, symlinks, and other non-regular file types +# so the local path cannot redirect writes back into the read-only tree. if (-e $dest || -l $dest) { if (-f $dest && !-l $dest) { - &flush_logrotate_config_cache(); - return $dest; + if (&same_file($source, $dest)) { + &error(&text('save_evendorwrite', "". + &html_escape($source)."")); + } + else { + &flush_logrotate_config_cache(); + return $dest; + } } &error(&text('save_eoverride', "". &html_escape($dest)."")); @@ -428,8 +435,8 @@ return undef; } # save_directive(&parent, &old|name, &new, [indent]) -# Update a single entry in the config, identified by either name or -# the direcctive being replaced +# Updates one entry identified by either its name or parsed object. Vendor +# entries are transparently copied and rebound to their writable overrides. sub save_directive { my ($parent, $oldv, $newv, $indent) = @_; @@ -439,27 +446,100 @@ my $new = !defined($newv) ? undef : ref($newv) ? $newv : { 'name' => $old ? $old->{'name'} : $oldv, 'value' => $newv }; -# Refuse direct vendor writes even if a caller forgets to materialize the -# local copy first. New log sections may still be written to their explicit -# local file while the global defaults continue to come from the vendor file. +# Find the vendor file behind this write. Existing directives use their own +# file, while additions use the parent section or the effective main config. my $vendor_file; -if ($old) { - my $shadowed_vendor = &get_vendor_config_file($old->{'file'}); - if (&is_vendor_main_config($old->{'file'}) || - &is_vendor_config_file($old->{'file'})) { - $vendor_file = $old->{'file'}; +my $write_file = $old ? $old->{'file'} : $parent->{'file'}; +if ($write_file) { + my $shadowed_vendor = &get_vendor_config_file($write_file); + if (&is_vendor_main_config($write_file) || + &is_vendor_config_file($write_file)) { + $vendor_file = $write_file; } elsif ($shadowed_vendor && - &same_file($old->{'file'}, $shadowed_vendor)) { + &same_file($write_file, $shadowed_vendor)) { $vendor_file = $shadowed_vendor; } } -elsif (!$old && $new && !$new->{'members'} && $parent->{'global'} && +if (!$vendor_file && !$old && $new && !$new->{'members'} && + $parent->{'global'} && &is_vendor_main_config(&get_main_config_file())) { $vendor_file = &get_main_config_file(); } -&error(&text('save_evendorwrite', - "".&html_escape($vendor_file)."")) if ($vendor_file); + +# Materialize a writable override, then replace stale parsed references with +# their identical positions in the newly-parsed local configuration. Exact +# copying keeps both top-level and member indexes stable across this reparse. +if ($vendor_file) { + my $parent_global = $parent->{'global'}; + my $parent_index = $parent->{'index'}; + my $old_index = $old ? $old->{'index'} : undef; + my $old_line = $old ? $old->{'line'} : undef; + my $old_ref = ref($oldv) ? $oldv : undef; + my $local_file = &is_vendor_main_config($vendor_file) ? + &ensure_local_main_config() : + &ensure_local_config_override($vendor_file); + if (!$local_file || &same_file($local_file, $vendor_file)) { + &error(&text('save_evendorwrite', + "".&html_escape($vendor_file)."")); + } + my $fresh_root = &get_config_parent(); + my $fresh_parent = $parent_global ? $fresh_root : + $fresh_root->{'members'}->[$parent_index]; + + # Fail closed if concurrent configuration changes made the saved indexes + # unsafe to reuse. This must never fall through to the vendor line cache. + if (!$fresh_parent || + (!$parent_global && + (!&same_file($fresh_parent->{'file'}, $local_file) || + !$fresh_parent->{'members'}))) { + &error(&text('save_evendorwrite', + "".&html_escape($vendor_file)."")); + } + + # Preserve the caller's parent object identity so later saves, explicit + # flushes, and unlocks all refer to the new writable local file. + %$parent = %$fresh_parent; + if ($parent_global) { + $get_config_parent_cache = $parent; + $fresh_root = $parent; + } + else { + $fresh_root->{'members'}->[$parent_index] = $parent; + } + $conf = $parent->{'members'}; + + # A referenced old object may also contain the caller's pending edits, as + # when Virtualmin changes a section name and passes the same object twice. + # Keep its data and identity, but rebase all of its file ownership locally. + if ($old) { + my $fresh_old = $conf->[$old_index]; + if (!$fresh_old || $fresh_old->{'line'} != $old_line || + !&same_file($fresh_old->{'file'}, $local_file)) { + &error(&text('save_evendorwrite', + "".&html_escape($vendor_file)."")); + } + if ($old_ref) { + $old_ref->{'line'} = $fresh_old->{'line'}; + $old_ref->{'eline'} = $fresh_old->{'eline'}; + $old_ref->{'index'} = $fresh_old->{'index'}; + my @objects = ($old_ref); + while (@objects) { + my $object = shift(@objects); + $object->{'file'} = $local_file + if ($object->{'file'} && + &same_file($object->{'file'}, $vendor_file)); + push(@objects, @{$object->{'members'}}) + if ($object->{'members'}); + } + $conf->[$old_index] = $old_ref; + $old = $old_ref; + } + else { + $old = $fresh_old; + } + } + } my $lref = &read_file_lines($old ? $old->{'file'} : $parent->{'file'}); my @lines = &directive_lines($new, $indent) if ($new); diff --git a/logrotate/t/run-tests.t b/logrotate/t/run-tests.t index 11b0bcc2d..b0ac610de 100644 --- a/logrotate/t/run-tests.t +++ b/logrotate/t/run-tests.t @@ -162,22 +162,24 @@ is_deeply(log_names($config), [ '/var/log/vendor-main.log' ], is_deeply($files, [ $vendor_main_file ], 'file cache excludes external directories when scanning is disabled'); -# The low-level writer must fail closed if a caller skips copy-on-write. -{ -no warnings qw(once redefine); -local *main::error = sub { die $_[0]; }; -eval { - main::save_directive(main::get_config_parent(), 'weekly', ''); - }; -like($@, qr/Refusing to modify vendor configuration/, - 'direct writes to vendor configuration are rejected'); -} +# The public writer performs copy-on-write itself, so API consumers do not +# need to know whether the effective main configuration came from /usr/etc. +my $vendor_parent = main::get_config_parent(); +main::save_directive($vendor_parent, 'weekly', ''); +main::flush_file_lines($local_main_file); +is(read_text($local_main_file), $vendor_main_text, + 'direct main-config writes automatically create a local copy'); +is(read_text($vendor_main_file), $vendor_main_text, + 'automatic main-config copying leaves the vendor file unchanged'); +is(main::find('weekly', $vendor_parent->{'members'})->{'file'}, + $local_main_file, + 'the caller parent is rebound to the writable main config'); -# Editing global options materializes an exact local copy of the vendor main. +# Repeated preparation is harmless once the local main override exists. $main::config{'scan_add_file'} = 1; clear_config_cache(); is(main::ensure_local_main_config(), $local_main_file, - 'editing the vendor main config creates a local main config'); + 'an existing local main config remains the write target'); is(read_text($local_main_file), $vendor_main_text, 'local main config starts as an exact vendor copy'); is(read_text($vendor_main_file), $vendor_main_text, @@ -185,12 +187,18 @@ is(read_text($vendor_main_file), $vendor_main_text, is(main::get_main_config_file(), $local_main_file, 'local main config takes precedence after it is created'); -# Editing a nested vendor drop-in copies the whole source file to the same -# relative path in the local tree and immediately switches parser ownership. +# A nested save through the public API copies the whole vendor drop-in to the +# same relative local path and immediately switches the caller's ownership. my $vendor_dropin = "$vendor_add_dir/deep/vendor"; my $local_dropin = "$local_add_dir/deep/vendor"; -is(main::ensure_local_config_override($vendor_dropin), $local_dropin, - 'editing a vendor drop-in creates its matching local override'); +($config, undef, $files) = main::get_config(); +my ($deep_log) = grep { $_->{'members'} && + $_->{'name'}->[0] eq '/var/log/deep-vendor.log' } + @$config; +main::save_directive($deep_log, 'monthly', '', "\t"); +main::flush_file_lines($local_dropin); +is($deep_log->{'file'}, $local_dropin, + 'nested vendor writes transparently rebind the caller locally'); is(read_text($local_dropin), read_text($vendor_dropin), 'local drop-in starts as an exact copy of the whole vendor file'); is(main::get_local_override_file($vendor_dropin), $local_dropin, @@ -199,12 +207,29 @@ is(main::get_vendor_config_file($local_dropin), $vendor_dropin, 'local override maps back to the shadowed vendor file'); ($config, undef, $files) = main::get_config(); -my ($deep_log) = grep { $_->{'members'} && - $_->{'name'}->[0] eq '/var/log/deep-vendor.log' } - @$config; +($deep_log) = grep { $_->{'members'} && + $_->{'name'}->[0] eq '/var/log/deep-vendor.log' } + @$config; is($deep_log->{'file'}, $local_dropin, 'parser switches to the local copy after an override is created'); +# Whole-section callers may mutate and pass the same parsed object as both +# old and new. Preserve those edits while changing its file ownership. +my $vendor_one = "$vendor_add_dir/one"; +my $local_one = "$local_add_dir/one"; +my ($one_log) = grep { $_->{'members'} && + $_->{'name'}->[0] eq '/var/log/vendor-one.log' } + @$config; +push(@{$one_log->{'name'}}, '/var/log/vendor-one-extra.log'); +main::save_directive(main::get_config_parent(), $one_log, $one_log); +main::flush_file_lines($local_one); +is($one_log->{'file'}, $local_one, + 'whole-section saves preserve the caller object and move it locally'); +like(read_text($local_one), qr{/var/log/vendor-one-extra\.log}, + 'whole-section saves preserve pending caller edits'); +unlike(read_text($vendor_one), qr{/var/log/vendor-one-extra\.log}, + 'whole-section saves never change the vendor source'); + # An empty local file must remain both effective and backup-visible because # its existence is what prevents the vendor file from becoming active again. write_text($local_dropin, ''); @@ -250,4 +275,21 @@ is_deeply([ main::get_add_file_configs() ], [ "$edge_local_dir/linked" ], 'local existing path overrides the matching vendor file'); } +# A regular local path must still be rejected when it is a hard link to its +# vendor source, because otherwise an apparently local write would alter /usr. +my $hardlink_dir = tempdir(CLEANUP => 1); +my $hardlink_vendor = "$hardlink_dir/vendor"; +my $hardlink_local = "$hardlink_dir/local"; +write_text($hardlink_vendor, "vendor\n"); +link($hardlink_vendor, $hardlink_local) or + die "link $hardlink_local: $!"; +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { main::copy_vendor_config($hardlink_vendor, $hardlink_local); }; +ok($@, 'a hard-linked local override is rejected'); +} +is(read_text($hardlink_vendor), "vendor\n", + 'rejecting a hard-linked override leaves the vendor source unchanged'); + done_testing(); From 07f0ddfb6dcf4a7bd2cf010d9dff0f2e612eca9b Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Mon, 10 Aug 2026 21:37:11 +0200 Subject: [PATCH 3/4] Add shared test file helpers --- logrotate/t/run-tests.t | 30 ++++++------------------------ t/test-lib.pl | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 24 deletions(-) create mode 100644 t/test-lib.pl diff --git a/logrotate/t/run-tests.t b/logrotate/t/run-tests.t index b0ac610de..7e86a599c 100644 --- a/logrotate/t/run-tests.t +++ b/logrotate/t/run-tests.t @@ -7,9 +7,13 @@ use File::Basename qw(dirname); use File::Path qw(make_path); use File::Temp qw(tempdir); -# Build an isolated openSUSE-style /etc and /usr/etc configuration layout. -my $module_dir = abs_path(dirname(abs_path($0))."/.."); +# Locate the repository and load its common test helpers. +my $test_dir = dirname(abs_path($0)); +my $module_dir = abs_path("$test_dir/.."); my $root_dir = abs_path("$module_dir/.."); +require "$root_dir/t/test-lib.pl"; + +# Build an isolated openSUSE-style /etc and /usr/etc configuration layout. my $config_dir = tempdir(CLEANUP => 1); my $var_dir = tempdir(CLEANUP => 1); my $fixture_dir = tempdir(CLEANUP => 1); @@ -22,28 +26,6 @@ make_path("$config_dir/logrotate", $local_add_dir, "$local_add_dir/nested", "$vendor_add_dir/deep", "$vendor_add_dir/nested", dirname($wrapper)); -# write_text(file, contents) -# Writes a text fixture and fails the test immediately on an I/O error -sub write_text -{ -my ($file, $text) = @_; -open(my $fh, ">", $file) or die "open $file: $!"; -print $fh $text; -close($fh) or die "close $file: $!"; -} - -# read_text(file) -# Returns the complete contents of a text fixture -sub read_text -{ -my ($file) = @_; -open(my $fh, "<", $file) or die "open $file: $!"; -local $/; -my $text = <$fh>; -close($fh) or die "close $file: $!"; -return $text; -} - # Populate both trees with vendor-only, local-only, nested, and overridden # files so the fixture exercises the wrapper's key overlay rules. my $vendor_main_text = diff --git a/t/test-lib.pl b/t/test-lib.pl new file mode 100644 index 000000000..d3611e8ee --- /dev/null +++ b/t/test-lib.pl @@ -0,0 +1,28 @@ +# Common helpers for Webmin tests. + +use strict; +use warnings; + +# write_text(file, contents) +# Writes a text fixture and fails the test immediately on an I/O error +sub write_text +{ +my ($file, $text) = @_; +open(my $fh, ">", $file) or die "open $file: $!"; +print $fh $text; +close($fh) or die "close $file: $!"; +} + +# read_text(file) +# Returns the complete contents of a text fixture +sub read_text +{ +my ($file) = @_; +open(my $fh, "<", $file) or die "open $file: $!"; +local $/; +my $text = <$fh>; +close($fh) or die "close $file: $!"; +return $text; +} + +1; From 5969c33e060c042adbcd7c722aba6e8d2eb76116 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Tue, 11 Aug 2026 20:24:38 +0200 Subject: [PATCH 4/4] Fix logrotate vendor override write handling --- logrotate/logrotate-lib.pl | 108 ++++++----------------- logrotate/save_log.cgi | 35 ++++++-- logrotate/t/run-tests.t | 176 +++++++++++++++++++++++++++++-------- 3 files changed, 192 insertions(+), 127 deletions(-) diff --git a/logrotate/logrotate-lib.pl b/logrotate/logrotate-lib.pl index b809543a8..c4689a03c 100755 --- a/logrotate/logrotate-lib.pl +++ b/logrotate/logrotate-lib.pl @@ -435,8 +435,7 @@ return undef; } # save_directive(&parent, &old|name, &new, [indent]) -# Updates one entry identified by either its name or parsed object. Vendor -# entries are transparently copied and rebound to their writable overrides. +# Updates one entry identified by either its name or parsed object sub save_directive { my ($parent, $oldv, $newv, $indent) = @_; @@ -446,10 +445,22 @@ my $new = !defined($newv) ? undef : ref($newv) ? $newv : { 'name' => $old ? $old->{'name'} : $oldv, 'value' => $newv }; -# Find the vendor file behind this write. Existing directives use their own -# file, while additions use the parent section or the effective main config. +# Deleting an entry that is already absent is a true no-op. In particular, +# do not put a missing local main config into the writable line cache. +return if (!$old && !$new); + +# Find the file behind this write. Existing directives use their own file, +# new sections may name a separate file, and other additions use the parent +# section or effective main config. my $vendor_file; -my $write_file = $old ? $old->{'file'} : $parent->{'file'}; +my $write_file = $parent->{'file'}; +if ($old) { + $write_file = $old->{'file'}; + } +elsif ($new && $new->{'file'} && + !($parent->{'global'} && !$new->{'members'})) { + $write_file = $new->{'file'}; + } if ($write_file) { my $shadowed_vendor = &get_vendor_config_file($write_file); if (&is_vendor_main_config($write_file) || @@ -457,91 +468,24 @@ if ($write_file) { $vendor_file = $write_file; } elsif ($shadowed_vendor && - &same_file($write_file, $shadowed_vendor)) { + (!-f $write_file || -l $write_file || + &same_file($write_file, $shadowed_vendor))) { $vendor_file = $shadowed_vendor; } } -if (!$vendor_file && !$old && $new && !$new->{'members'} && - $parent->{'global'} && +if (!$vendor_file && !$old && $parent->{'global'} && + &same_file($write_file, $parent->{'file'}) && &is_vendor_main_config(&get_main_config_file())) { $vendor_file = &get_main_config_file(); } -# Materialize a writable override, then replace stale parsed references with -# their identical positions in the newly-parsed local configuration. Exact -# copying keeps both top-level and member indexes stable across this reparse. -if ($vendor_file) { - my $parent_global = $parent->{'global'}; - my $parent_index = $parent->{'index'}; - my $old_index = $old ? $old->{'index'} : undef; - my $old_line = $old ? $old->{'line'} : undef; - my $old_ref = ref($oldv) ? $oldv : undef; - my $local_file = &is_vendor_main_config($vendor_file) ? - &ensure_local_main_config() : - &ensure_local_config_override($vendor_file); - if (!$local_file || &same_file($local_file, $vendor_file)) { - &error(&text('save_evendorwrite', - "".&html_escape($vendor_file)."")); - } - my $fresh_root = &get_config_parent(); - my $fresh_parent = $parent_global ? $fresh_root : - $fresh_root->{'members'}->[$parent_index]; +# Copying changes which file owns the parsed objects, so callers must create +# and reload a local override before editing. Never write through a stale +# object that still points at the vendor tree. +&error(&text('save_evendorwrite', + "".&html_escape($vendor_file)."")) if ($vendor_file); - # Fail closed if concurrent configuration changes made the saved indexes - # unsafe to reuse. This must never fall through to the vendor line cache. - if (!$fresh_parent || - (!$parent_global && - (!&same_file($fresh_parent->{'file'}, $local_file) || - !$fresh_parent->{'members'}))) { - &error(&text('save_evendorwrite', - "".&html_escape($vendor_file)."")); - } - - # Preserve the caller's parent object identity so later saves, explicit - # flushes, and unlocks all refer to the new writable local file. - %$parent = %$fresh_parent; - if ($parent_global) { - $get_config_parent_cache = $parent; - $fresh_root = $parent; - } - else { - $fresh_root->{'members'}->[$parent_index] = $parent; - } - $conf = $parent->{'members'}; - - # A referenced old object may also contain the caller's pending edits, as - # when Virtualmin changes a section name and passes the same object twice. - # Keep its data and identity, but rebase all of its file ownership locally. - if ($old) { - my $fresh_old = $conf->[$old_index]; - if (!$fresh_old || $fresh_old->{'line'} != $old_line || - !&same_file($fresh_old->{'file'}, $local_file)) { - &error(&text('save_evendorwrite', - "".&html_escape($vendor_file)."")); - } - if ($old_ref) { - $old_ref->{'line'} = $fresh_old->{'line'}; - $old_ref->{'eline'} = $fresh_old->{'eline'}; - $old_ref->{'index'} = $fresh_old->{'index'}; - my @objects = ($old_ref); - while (@objects) { - my $object = shift(@objects); - $object->{'file'} = $local_file - if ($object->{'file'} && - &same_file($object->{'file'}, $vendor_file)); - push(@objects, @{$object->{'members'}}) - if ($object->{'members'}); - } - $conf->[$old_index] = $old_ref; - $old = $old_ref; - } - else { - $old = $fresh_old; - } - } - } - -my $lref = &read_file_lines($old ? $old->{'file'} : $parent->{'file'}); +my $lref = &read_file_lines($write_file); my @lines = &directive_lines($new, $indent) if ($new); my $gparent = &get_config_parent(); if ($old && $new) { diff --git a/logrotate/save_log.cgi b/logrotate/save_log.cgi index f3691bdee..0be324331 100755 --- a/logrotate/save_log.cgi +++ b/logrotate/save_log.cgi @@ -5,6 +5,32 @@ require './logrotate-lib.pl'; &ReadParse(); +# Resolve a new section's destination before loading any parsed objects. If +# its relative name already exists in the vendor tree, materialize the whole +# local override before appending the new section. +@files = split(/\s+/, $in{'file'}); +if ($in{'new'} || + (!$in{'global'} && !$in{'delete'} && !$in{'now'})) { + &error_setup($text{'save_err'}); + foreach $f (@files) { + $f =~ /^\/\S+$/ || &error($text{'save_efile'}); + } + @files || &error($text{'save_enofiles'}); + $in{'file'} =~ s/\r//g; + } +if ($in{'new'}) { + $cfilename = $files[0] =~ /\/([^\/]+)$/ ? $1 : undef; + $new_config_file = &get_add_file($cfilename); + $vendor_file = &get_vendor_config_file($new_config_file); + if ($vendor_file) { + &ensure_local_config_override($vendor_file); + } + elsif (&same_file($new_config_file, $config{'logrotate_conf'}) && + &is_vendor_main_config(&get_main_config_file())) { + &ensure_local_main_config(); + } + } + # On systems with vendor configuration below /usr, create the writable local # main config before changing global options. The parent object intentionally # keeps this local path as its write destination. @@ -20,16 +46,14 @@ if (!$in{'global'} && !$in{'new'} && !$in{'now'} && $parent = &get_config_parent(); $conf = $parent->{'members'}; } -@files = split(/\s+/, $in{'file'}); if ($in{'global'}) { # Editing the global options $log = $parent; } elsif ($in{'new'}) { # Adding a new section - $cfilename = $files[0] =~ /\/([^\/]+)$/ ? $1 : undef; $log = { 'members' => [ ], - 'file' => &get_add_file($cfilename) }; + 'file' => $new_config_file }; $logfile = $in{'file'}; } else { @@ -66,11 +90,6 @@ else { &lock_file($log->{'file'}); &error_setup($text{'save_err'}); if (!$in{'global'}) { - foreach $f (@files) { - $f =~ /^\/\S+$/ || &error($text{'save_efile'}); - } - @files || &error($text{'save_enofiles'}); - $in{'file'} =~ s/\r//g; $log->{'name'} = [ split(/\n/, $in{'file'}) ]; } diff --git a/logrotate/t/run-tests.t b/logrotate/t/run-tests.t index 7e86a599c..9ab92d237 100644 --- a/logrotate/t/run-tests.t +++ b/logrotate/t/run-tests.t @@ -144,24 +144,96 @@ is_deeply(log_names($config), [ '/var/log/vendor-main.log' ], is_deeply($files, [ $vendor_main_file ], 'file cache excludes external directories when scanning is disabled'); -# The public writer performs copy-on-write itself, so API consumers do not -# need to know whether the effective main configuration came from /usr/etc. -my $vendor_parent = main::get_config_parent(); -main::save_directive($vendor_parent, 'weekly', ''); -main::flush_file_lines($local_main_file); -is(read_text($local_main_file), $vendor_main_text, - 'direct main-config writes automatically create a local copy'); -is(read_text($vendor_main_file), $vendor_main_text, - 'automatic main-config copying leaves the vendor file unchanged'); -is(main::find('weekly', $vendor_parent->{'members'})->{'file'}, - $local_main_file, - 'the caller parent is rebound to the writable main config'); +# The low-level writer must fail closed if a caller skips copy-on-write. +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { + main::save_directive(main::get_config_parent(), 'weekly', ''); + }; +like($@, qr/Refusing to modify vendor configuration/, + 'direct writes to the vendor main configuration are rejected'); +} -# Repeated preparation is harmless once the local main override exists. +# Deleting an already-absent option is a no-op and must not cache an empty +# local main file that a later unscoped flush could accidentally create. +main::save_directive(main::get_config_parent(), + 'missing-vendor-option', undef); +main::flush_file_lines(); +ok(!-e $local_main_file, + 'missing global option deletion leaves the local main config absent'); + +# A new section with an explicit vendor destination must also fail closed. +my $vendor_target = "$vendor_add_dir/one"; +my $vendor_target_text = read_text($vendor_target); +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { + main::save_directive(main::get_config_parent(), undef, + { 'file' => $vendor_target, + 'name' => [ '/var/log/unsafe-vendor-write.log' ], + 'members' => [ ] }); + }; +like($@, qr/Refusing to modify vendor configuration/, + 'new sections cannot target a vendor drop-in directly'); +} +is(read_text($vendor_target), $vendor_target_text, + 'rejecting a new vendor section leaves its destination unchanged'); + +# A section without its own file would create an incomplete local main config. +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { + main::save_directive(main::get_config_parent(), undef, + { 'name' => [ '/var/log/unsafe-main-write.log' ], + 'members' => [ ] }); + }; +like($@, qr/Refusing to modify vendor configuration/, + 'new sections cannot replace the vendor main config implicitly'); +} +ok(!-e $local_main_file, + 'rejecting an implicit main write does not create a partial override'); + +# Adding a fresh local drop-in must not put the absent local main in the line +# cache, because the normal unscoped flush would then create it as an empty +# file and hide the complete vendor main configuration. +my $new_local_dropin = "$local_add_dir/new-local"; +main::save_directive(main::get_config_parent(), undef, + { 'file' => $new_local_dropin, + 'name' => [ '/var/log/new-local.log' ], + 'members' => [ { 'name' => 'weekly' } ] }); +main::flush_file_lines(); +ok(-f $new_local_dropin, + 'new sections are written to their explicit local drop-in'); +ok(!-e $local_main_file, + 'adding a local drop-in does not create an empty local main config'); +is(read_text($vendor_main_file), $vendor_main_text, + 'adding a local drop-in leaves the vendor main config unchanged'); + +# A missing local file cannot safely replace a whole same-named vendor file. +my $missing_local_override = "$local_add_dir/one"; +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { + main::save_directive(main::get_config_parent(), undef, + { 'file' => $missing_local_override, + 'name' => [ '/var/log/incomplete-override.log' ], + 'members' => [ ] }); + }; +like($@, qr/Refusing to modify vendor configuration/, + 'new sections cannot create incomplete vendor overrides'); +} +ok(!-e $missing_local_override, + 'rejecting an incomplete override leaves its local path absent'); + +# Editing global options materializes an exact local copy before parsing. $main::config{'scan_add_file'} = 1; clear_config_cache(); is(main::ensure_local_main_config(), $local_main_file, - 'an existing local main config remains the write target'); + 'editing the vendor main config creates a local main config'); is(read_text($local_main_file), $vendor_main_text, 'local main config starts as an exact vendor copy'); is(read_text($vendor_main_file), $vendor_main_text, @@ -169,18 +241,40 @@ is(read_text($vendor_main_file), $vendor_main_text, is(main::get_main_config_file(), $local_main_file, 'local main config takes precedence after it is created'); -# A nested save through the public API copies the whole vendor drop-in to the -# same relative local path and immediately switches the caller's ownership. +# A new section may be appended after the same-named vendor file has been +# copied in full, which is the preflight performed by save_log.cgi. +is(main::ensure_local_config_override($vendor_target), + $missing_local_override, + 'new-section preflight creates the complete local override'); +my $prepared_parent = main::get_config_parent(); +main::save_directive($prepared_parent, undef, + { 'file' => $missing_local_override, + 'name' => [ '/var/log/appended-local.log' ], + 'members' => [ { 'name' => 'weekly' } ] }); +main::flush_file_lines($missing_local_override); +like(read_text($missing_local_override), qr{/var/log/vendor-one\.log}, + 'prepared override retains the original vendor section'); +like(read_text($missing_local_override), qr{/var/log/appended-local\.log}, + 'prepared override receives the new local section'); +is(read_text($vendor_target), $vendor_target_text, + 'appending locally leaves the same-named vendor file unchanged'); + +# Editing a vendor drop-in must also be prepared before parsed objects change. my $vendor_dropin = "$vendor_add_dir/deep/vendor"; my $local_dropin = "$local_add_dir/deep/vendor"; ($config, undef, $files) = main::get_config(); my ($deep_log) = grep { $_->{'members'} && $_->{'name'}->[0] eq '/var/log/deep-vendor.log' } @$config; -main::save_directive($deep_log, 'monthly', '', "\t"); -main::flush_file_lines($local_dropin); -is($deep_log->{'file'}, $local_dropin, - 'nested vendor writes transparently rebind the caller locally'); +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { main::save_directive($deep_log, 'monthly', '', "\t"); }; +like($@, qr/Refusing to modify vendor configuration/, + 'direct writes to a vendor drop-in are rejected'); +} +is(main::ensure_local_config_override($vendor_dropin), $local_dropin, + 'editing a vendor drop-in creates its matching local override'); is(read_text($local_dropin), read_text($vendor_dropin), 'local drop-in starts as an exact copy of the whole vendor file'); is(main::get_local_override_file($vendor_dropin), $local_dropin, @@ -194,23 +288,12 @@ is(main::get_vendor_config_file($local_dropin), $vendor_dropin, @$config; is($deep_log->{'file'}, $local_dropin, 'parser switches to the local copy after an override is created'); - -# Whole-section callers may mutate and pass the same parsed object as both -# old and new. Preserve those edits while changing its file ownership. -my $vendor_one = "$vendor_add_dir/one"; -my $local_one = "$local_add_dir/one"; -my ($one_log) = grep { $_->{'members'} && - $_->{'name'}->[0] eq '/var/log/vendor-one.log' } - @$config; -push(@{$one_log->{'name'}}, '/var/log/vendor-one-extra.log'); -main::save_directive(main::get_config_parent(), $one_log, $one_log); -main::flush_file_lines($local_one); -is($one_log->{'file'}, $local_one, - 'whole-section saves preserve the caller object and move it locally'); -like(read_text($local_one), qr{/var/log/vendor-one-extra\.log}, - 'whole-section saves preserve pending caller edits'); -unlike(read_text($vendor_one), qr{/var/log/vendor-one-extra\.log}, - 'whole-section saves never change the vendor source'); +main::save_directive($deep_log, 'monthly', undef, "\t"); +main::flush_file_lines($local_dropin); +unlike(read_text($local_dropin), qr/^\s*monthly\s*$/m, + 'prepared drop-in can be changed through its local override'); +like(read_text($vendor_dropin), qr/^\s*monthly\s*$/m, + 'changing the local override leaves the vendor drop-in unchanged'); # An empty local file must remain both effective and backup-visible because # its existence is what prevents the vendor file from becoming active again. @@ -255,6 +338,25 @@ local $main::config{'add_file'} = $edge_local_dir; local $main::config{'vendor_add_file'} = $edge_vendor_dir; is_deeply([ main::get_add_file_configs() ], [ "$edge_local_dir/linked" ], 'local existing path overrides the matching vendor file'); + +# Discovery follows the wrapper's existence rule, but editing must not follow +# a local symlink when it shadows a same-named vendor configuration. +{ +no warnings qw(once redefine); +local *main::error = sub { die $_[0]; }; +eval { + main::save_directive( + { 'members' => [ ], 'file' => "$edge_dir/parent" }, + undef, + { 'file' => "$edge_local_dir/linked", + 'name' => [ '/var/log/symlink-write.log' ], + 'members' => [ ] }); + }; +like($@, qr/Refusing to modify vendor configuration/, + 'local symlink overrides are rejected for editing'); +} +is(read_text($edge_target), "local\n", + 'rejecting a symlink override leaves its target unchanged'); } # A regular local path must still be rejected when it is a hard link to its