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