From efe1dddca886a00452ec57e44aa04afd67186fa6 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Thu, 17 Sep 2026 01:46:50 +0200 Subject: [PATCH] Fix to change Apache comments to use directive fields https://github.com/webmin/webmin/pull/2847#issuecomment-5705632150 --- apache/apache-lib.pl | 121 +++++++++++++++++++--------------- apache/t/directive-comments.t | 116 +++++++++++++++++++++++++++++--- 2 files changed, 176 insertions(+), 61 deletions(-) diff --git a/apache/apache-lib.pl b/apache/apache-lib.pl index f7e364a3b..d1e5ce331 100755 --- a/apache/apache-lib.pl +++ b/apache/apache-lib.pl @@ -126,10 +126,10 @@ if (&read_file($site_file, \%site)) { # value - Value (possibly with spaces) # members - For type 1, a reference to the array of members # indent - Number of spaces before the name -# comment - Full text for a comment stored as a dummy directive +# comments - Full comment lines immediately before this directive sub parse_config_file { -local($fh, @rv, $line, %dummy); +local($fh, @rv, $line, %dummy, @comments); $fh = $_[0]; $dummy{'line'} = $dummy{'eline'} = $_[1]-1; $dummy{'file'} = $_[2]; @@ -147,19 +147,10 @@ foreach my $d (&get_httpd_defines()) { } while($line = <$fh>) { $line =~ s/\r|\n//g; - if ($line =~ /^(\s*)(#.*)$/) { - # Keep comments in the structure so block rewrites preserve them - local(%dir); - %dir = ('line', $_[1], - 'eline', $_[1], - 'file', $_[2], - 'type', 0, - 'name', 'dummy', - 'comment', $2); - local $indent = $1; - $indent =~ s/\t/ /g; - $dir{'indent'} = length($indent); - push(@rv, \%dir); + if ($line =~ /^\s*#/) { + # Attach comments to the next directive without reusing the + # dummy entry that marks the start of this block + push(@comments, $line); $_[1]++; } elsif ($line =~ /^\s*<\/(\S+)\s*(.*)>/) { @@ -193,10 +184,13 @@ while($line = <$fh>) { $not && !$httpd_modules{$mpmmod} ) { # use the directives.. - push(@rv, { 'line', $oldline, - 'eline', $oldline, - 'file', $_[2], - 'name', "" }); + local %open = ( 'line', $oldline, + 'eline', $oldline, + 'file', $_[2], + 'name', "" ); + $open{'comments'} = [ @comments ] if (@comments); + @comments = ( ); + push(@rv, \%open); push(@rv, @dirs); push(@rv, { 'line', $_[1]-1, 'eline', $_[1]-1, @@ -214,10 +208,13 @@ while($line = <$fh>) { if (!$not && defined($defs{$def}) || $not && !defined($defs{$def})) { # use the directives.. - push(@rv, { 'line', $oldline, - 'eline', $oldline, - 'file', $_[2], - 'name', "" }); + local %open = ( 'line', $oldline, + 'eline', $oldline, + 'file', $_[2], + 'name', "" ); + $open{'comments'} = [ @comments ] if (@comments); + @comments = ( ); + push(@rv, \%open); push(@rv, @dirs); push(@rv, { 'line', $_[1]-1, 'eline', $_[1]-1, @@ -262,10 +259,13 @@ while($line = <$fh>) { $match = !$match if ($not); if ($match) { # use the directives.. - push(@rv, { 'line', $oldline, - 'eline', $oldline, - 'file', $_[2], - 'name', "" }); + local %open = ( 'line', $oldline, + 'eline', $oldline, + 'file', $_[2], + 'name', "" ); + $open{'comments'} = [ @comments ] if (@comments); + @comments = ( ); + push(@rv, \%open); push(@rv, @dirs); push(@rv, { 'line', $_[1]-1, 'eline', $_[1]-1, @@ -291,6 +291,8 @@ while($line = <$fh>) { $dir{'eline'} = $_[1]-1; $indent =~ s/\t/ /g; $dir{'indent'} = length($indent); + $dir{'comments'} = [ @comments ] if (@comments); + @comments = ( ); push(@rv, \%dir); } elsif ($line =~ /^(\s*)(\S+)\s*(.*)$/) { @@ -324,11 +326,15 @@ while($line = <$fh>) { } } $dir{'words'} = &wsplit($dir{'value'}); + $dir{'comments'} = [ @comments ] if (@comments); + @comments = ( ); push(@rv, \%dir); $_[1]++; } else { - # blank or comment line + # Keep blank lines that separate a pending comment block from + # the directive it describes + push(@comments, $line) if (@comments); $_[1]++; } } @@ -651,9 +657,12 @@ for($i=0; $i<@old || $i<@{$_[1]}; $i++) { push(@files, $old[$i]->{'file'}); $idx = &indexof($old[$i], @{$_[2]}); splice(@{$_[2]}, $idx, 1); - $len = $old[$i]->{'eline'} - $old[$i]->{'line'} + 1; - splice(@$lref, $old[$i]->{'line'}, $len); - &renumber($_[3], $old[$i]->{'line'}, $old[$i]->{'file'}, -$len); + local $comments = $old[$i]->{'comments'} ? + scalar(@{$old[$i]->{'comments'}}) : 0; + local $start = $old[$i]->{'line'}-$comments; + $len = $old[$i]->{'eline'}-$start+1; + splice(@$lref, $start, $len); + &renumber($_[3], $start, $old[$i]->{'file'}, -$len); } else { # just changing the value @@ -688,9 +697,16 @@ return if (!$olddir && !$newdir); # Nothing to do local $file = $olddir ? $olddir->{'file'} : $newdir->{'file'} ? $newdir->{'file'} : $pconf->[0]->{'file'}; local $lref = &read_file_lines($file); -local $oldlen = $olddir ? $olddir->{'eline'}-$olddir->{'line'}+1 : undef; +local $oldcomments = $olddir && $olddir->{'comments'} ? + scalar(@{$olddir->{'comments'}}) : 0; +local $oldline = $olddir ? $olddir->{'line'}-$oldcomments : undef; +local $oldlen = $olddir ? $olddir->{'eline'}-$oldline+1 : undef; local @newlines; if ($newdir) { + if ($olddir && !exists($newdir->{'comments'}) && $oldcomments) { + # A replacement inherits the comments attached to the old block + $newdir->{'comments'} = [ @{$olddir->{'comments'}} ]; + } my $isrc = $olddir ? $olddir : @$pconf ? $pconf->[0] : undef; if ($isrc) { @@ -704,7 +720,9 @@ if ($olddir && $newdir) { $newdir->{'words'} = &wsplit($newdir->{'value'}); if ($first) { # Just changing first and last line, like virtualhost IP - $lref->[$olddir->{'line'}] = $newlines[0]; + local $comments = $newdir->{'comments'} ? + scalar(@{$newdir->{'comments'}}) : 0; + $lref->[$olddir->{'line'}] = $newlines[$comments]; $lref->[$olddir->{'eline'}] = $newlines[$#newlines]; $olddir->{'name'} = $newdir->{'name'}; $olddir->{'value'} = $newdir->{'value'}; @@ -717,9 +735,11 @@ if ($olddir && $newdir) { local $idx = &indexof($olddir, @$pconf); $pconf->[$idx] = $newdir if ($idx >= 0); $newdir->{'file'} = $olddir->{'file'}; - $newdir->{'line'} = $olddir->{'line'}; - $newdir->{'eline'} = $olddir->{'line'}+scalar(@newlines)-1; - splice(@$lref, $olddir->{'line'}, $oldlen, @newlines); + local $comments = $newdir->{'comments'} ? + scalar(@{$newdir->{'comments'}}) : 0; + $newdir->{'line'} = $oldline+$comments; + $newdir->{'eline'} = $oldline+scalar(@newlines)-1; + splice(@$lref, $oldline, $oldlen, @newlines); # Update sub-directive lines and files too if ($newdir->{'type'}) { @@ -731,10 +751,10 @@ if ($olddir && $newdir) { } elsif ($olddir && !$newdir) { # Remove - splice(@$lref, $olddir->{'line'}, $oldlen); + splice(@$lref, $oldline, $oldlen); local $idx = &indexof($olddir, @$pconf); splice(@$pconf, $idx, 1) if ($idx >= 0); - &renumber($conf, $olddir->{'line'}, $olddir->{'file'}, -$oldlen); + &renumber($conf, $oldline, $olddir->{'file'}, -$oldlen); } elsif (!$olddir && $newdir) { # Add to file, at end of specific file or parent section @@ -754,7 +774,9 @@ elsif (!$olddir && $newdir) { } $newdir->{'words'} = &wsplit($newdir->{'value'}); $newdir->{'file'} = $file; - $newdir->{'line'} = $addline; + local $comments = $newdir->{'comments'} ? + scalar(@{$newdir->{'comments'}}) : 0; + $newdir->{'line'} = $addline+$comments; $newdir->{'eline'} = $addline + scalar(@newlines) - 1; &renumber($conf, $addline, $file, scalar(@newlines)); splice(@$pconf, $addpos, 0, $newdir); @@ -791,17 +813,17 @@ sub recursive_set_lines_files { my ($dirs, $line, $file) = @_; foreach my $dir (@$dirs) { + # A directive's line follows any comments attached to it + $line += scalar(@{$dir->{'comments'}}) if ($dir->{'comments'}); $dir->{'line'} = $line; $dir->{'file'} = $file; if ($dir->{'type'}) { - # Do sub-members too - &recursive_set_lines_files($dir->{'members'}, $line+1, $file); - $line += scalar(grep { $_->{'name'} ne 'dummy' || - defined($_->{'comment'}) } - @{$dir->{'members'}})+1; + # Continue after every line used by nested members + $line = &recursive_set_lines_files($dir->{'members'}, + $line+1, $file); } $dir->{'eline'} = $line; - $line++ if ($dir->{'name'} ne 'dummy' || defined($dir->{'comment'})); + $line++ if ($dir->{'name'} ne 'dummy'); } return $line; } @@ -1947,13 +1969,8 @@ sub directive_lines { my @rv; foreach my $d (@_) { - if ($d->{'name'} eq 'dummy') { - if (defined($d->{'comment'})) { - my $indent = (" " x $d->{'indent'}); - push(@rv, $indent.$d->{'comment'}); - } - next; - } + next if ($d->{'name'} eq 'dummy'); + push(@rv, @{$d->{'comments'}}) if ($d->{'comments'}); my $indent = (" " x $d->{'indent'}); if ($d->{'type'}) { push(@rv, $indent."<$d->{'name'} $d->{'value'}>"); diff --git a/apache/t/directive-comments.t b/apache/t/directive-comments.t index 19e2cd577..2deebd9d7 100644 --- a/apache/t/directive-comments.t +++ b/apache/t/directive-comments.t @@ -67,6 +67,7 @@ my $roundtrip_text = "# outer comment\n". "\n". " # nested comment\n". + "\n". " Require all granted\n". "\n"; write_text($roundtrip, $roundtrip_text); @@ -74,18 +75,31 @@ open(my $fh, '<', $roundtrip) || die "Failed to read $roundtrip: $!"; my $line = 0; my @roundtrip_config = main::parse_config_file($fh, $line, $roundtrip); close($fh) || die "Failed to close $roundtrip: $!"; +my ($directory) = + main::find_directive_struct('Directory', \@roundtrip_config); +my ($require) = + main::find_directive_struct('Require', $directory->{'members'}); +is_deeply($directory->{'comments'}, [ '# outer comment' ], + 'outer comment is attached to the directory'); +is_deeply($require->{'comments'}, [ ' # nested comment', '' ], + 'nested comment block is attached to the following directive'); +ok(!$roundtrip_config[0]->{'comments'} && + !$directory->{'members'}->[0]->{'comments'}, + 'dummy block markers do not store comments'); is(join("\n", main::directive_lines(@roundtrip_config))."\n", $roundtrip_text, 'comments survive parsing and serialization'); -# Rewriting and removing virtual hosts must retain comments in kept blocks and -# remove comments that belong to deleted blocks. +# Rewriting and removing virtual hosts must handle attached comments with them. my $vhosts_file = File::Spec->catfile($tmp, 'vhosts.conf'); my $first_vhost = + "# first virtual host\n". "\n". " # first comment\n". + "\n". " ServerName first.example\n". "\n"; my $second_vhost = + "# second virtual host\n". "\n". " # second comment\n". " ServerName second.example\n". @@ -98,19 +112,103 @@ close($fh) || die "Failed to close $vhosts_file: $!"; my @vhosts = main::find_directive_struct('VirtualHost', \@vhost_config); main::recursive_set_lines_files(\@vhost_config, 0, $vhosts_file); -is($vhosts[0]->{'eline'}, 3, 'comment is included in the block line count'); -is($vhosts[1]->{'line'}, 4, 'next block starts after the preserved comment'); +is($vhosts[0]->{'line'}, 1, 'block line follows its attached comment'); +is($vhosts[0]->{'eline'}, 5, 'member comment is included in block line count'); +is($vhosts[1]->{'line'}, 7, 'next block follows its attached comment'); -main::save_directive_struct($vhosts[0], $vhosts[0], +my %replacement = %{$vhosts[0]}; +delete($replacement{'comments'}); +$replacement{'value'} = '*:8080'; +my $rewritten_first = $first_vhost; +$rewritten_first =~ s/\*:80>/*:8080>/; +main::save_directive_struct($vhosts[0], \%replacement, \@vhost_config, \@vhost_config); main::flush_file_lines($vhosts_file); -is(read_text($vhosts_file), $first_vhost.$second_vhost, - 'comments survive a block rewrite'); +is(read_text($vhosts_file), $rewritten_first.$second_vhost, + 'replacement block preserves surrounding comments'); +is_deeply($replacement{'comments'}, [ '# first virtual host' ], + 'replacement block inherits attached comments'); main::save_directive_struct($vhosts[1], undef, \@vhost_config, \@vhost_config); main::flush_file_lines($vhosts_file); -is(read_text($vhosts_file), $first_vhost, - 'deleting a block removes only its comments'); +is(read_text($vhosts_file), + $rewritten_first, + 'deleting a block removes its attached comment'); + +# Normal directive edits preserve comments, while deletion removes their owner. +my $directives_file = File::Spec->catfile($tmp, 'directives.conf'); +my $directives_text = + "# listen comment\n". + "Listen 80\n". + "# server name comment\n". + "ServerName old.example\n"; +write_text($directives_file, $directives_text); +open($fh, '<', $directives_file) || + die "Failed to read $directives_file: $!"; +$line = 0; +my @directive_config = + main::parse_config_file($fh, $line, $directives_file); +close($fh) || die "Failed to close $directives_file: $!"; + +main::save_directive('ServerName', [ 'new.example' ], + \@directive_config, \@directive_config); +main::flush_file_lines($directives_file); +my $updated_directives = + "# listen comment\n". + "Listen 80\n". + "# server name comment\n". + "ServerName new.example\n"; +is(read_text($directives_file), $updated_directives, + 'comments survive a normal directive edit'); + +main::save_directive('Listen', [ ], \@directive_config, \@directive_config); +main::flush_file_lines($directives_file); +my $deleted_directive = + "# server name comment\n". + "ServerName new.example\n"; +is(read_text($directives_file), $deleted_directive, + 'deleting a directive removes its attached comment'); + +open($fh, '<', $directives_file) || + die "Failed to read $directives_file: $!"; +$line = 0; +@directive_config = main::parse_config_file($fh, $line, $directives_file); +close($fh) || die "Failed to close $directives_file: $!"; +my ($servername) = + main::find_directive_struct('ServerName', \@directive_config); +is_deeply($servername->{'comments'}, + [ '# server name comment' ], + 'remaining comment stays attached to its directive'); + +# Nested block positions must include comments at every level. +my $inner = { + 'name' => 'Files', 'value' => '*.php', 'type' => 1, + 'comments' => [ ' # files comment' ], + 'members' => [ + { 'name' => 'dummy', 'type' => 0 }, + { 'name' => 'Require', 'value' => 'all denied', + 'comments' => [ ' # access comment' ] }, + ], + }; +my $outer = { + 'name' => 'Directory', 'value' => '/srv/example', 'type' => 1, + 'members' => [ + { 'name' => 'dummy', 'type' => 0 }, + $inner, + { 'name' => 'Options', 'value' => 'Indexes' }, + ], + }; +my @nested = ($outer); +my @nested_lines = main::directive_lines(@nested); +my $next_line = main::recursive_set_lines_files(\@nested, 20, $vhosts_file); +is($inner->{'line'}, 22, 'nested block line follows its comment'); +is($inner->{'members'}->[1]->{'line'}, 24, + 'nested member line follows its comment'); +is($outer->{'members'}->[2]->{'line'}, 26, + 'directive after nested block has the correct line'); +is($outer->{'eline'}, 27, 'outer block ends after all nested lines'); +is($next_line, 20 + scalar(@nested_lines), + 'line count matches nested serialized output'); done_testing();