Re: [PATCH v2] gitweb: Fixes error handling when reading configuration

[Junio C Hamano <gitster@pobox.com>]

Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> writes:

> This patch fixes a possibility of a permission to access error go
> unnoticed.
>
> Perl uses two different variables to manage errors from a do. One
> is $@, which is set in this case when do is unable to compile the
> file. The other is $!, which is set in case do cannot read the file.
> By printing the value of $! I found out that it was set to Permission
> denied. Since the script does not currently test for $!, the error
> goes unnoticed.

Well explained how the current code behaves and why.

With my devil's advocate hat on, I suspect that the current
behaviour comes from the wish to see "file exists but unreadable"
and "the named file does not exist" behave the same way.  If you
pass the name of a configuration file that does not exist, however,
the codeblock to die does not trigger at all.  If a file does exist
but unreadable, to gitweb, it is just as good as having no file to
read configuration data from---either way, it cannot use anything
useful from the named file.  So returning silently, which is the
"bug" you are fixing, does not sound too bad.

I dunno.  Let's queue and see what others would say.

Thanks.

> Perl do block documentation: https://perldoc.perl.org/functions/do
>
> Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>
> ---
>  gitweb/gitweb.perl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index e66eb3d9ba..5d0122020f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -728,9 +728,11 @@ sub filter_and_validate_refs {
>  sub read_config_file {
>  	my $filename = shift;
>  	return unless defined $filename;
> -	# die if there are errors parsing config file
>  	if (-e $filename) {
>  		do $filename;
> +		# die if there is a problem accessing the file
> +		die $! if $!;
> +		# die if there are errors parsing config file
>  		die $@ if $@;
>  		return 1;
>  	}