Problem in gitweb.cgi

Problem in gitweb.cgi

[Marcelo Roberto Jimenez]

Hi,

I have recently run into a problem with gitweb that was triggered by
my distribution, OpenSUSE Tumbleweed. Due to a misconfiguration of the
application AppArmour, "git instaweb" was having problems in accessing
the configuration file "/etc/gitweb-common.conf". That was half of the
problem and has been reported downstream here:
https://bugzilla.suse.com/show_bug.cgi?id=1218664

The other half of the problem is in gitweb.cgi itself. There is a
logic to select which configuration file is going to be used. That
logic is ok, although a bit unclear from the documentation. Reading
the code I understood that $GITWEB_CONFIG_COMMON (usually
/etc/gitweb-common.conf) should always be read if it exists, and
$GITWEB_CONFIG_SYSTEM (usually /etc/gitweb.conf) will never be read if
$GITWEB_CONFIG has been read before.

The problem is that $GITWEB_CONFIG_COMMON was never read, even though
the code that reads it was being called before the code that reads the
other two files. As I said before, the problem was caused by a lack of
permission in AppArmour, but gitweb should have been able to catch the
error. By not signaling it properly, users get lost because the
problem is a little tricky to debug.

After some "printf" debugging, I converged to this routine, line 728
of gitweb.cgi:

# read and parse gitweb config file given by its parameter.
# returns true on success, false on recoverable error, allowing
# to chain this subroutine, using first file that exists.
# dies on errors during parsing config file, as it is unrecoverable.
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 $@;
                return 1;
        }
        return;
}

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.

I suppose a proper fix would be to put a line before the test for $@,
like "die $! if $!".

For the curious, I have a report explaining the full problem here, but
the part relevant to gitweb is in this e-mail:
https://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n

Best regards,
Marcelo.

[PATCH] gitweb: Fixes error handling when reading configuration

[Marcelo Roberto Jimenez]

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.

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..47577ec566 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;
 	}
-- 
2.43.0

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

[Marcelo Roberto Jimenez]

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.

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;
 	}

base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
-- 
2.43.0

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

[Junio C Hamano]

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;
>  	}

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

[Marcelo Roberto Jimenez]

Hi Junio,

Ok, I'll put on my devil's prosecutor's hat then :)

I am not sure you read the first message in this thread (I know, I
messed up, sorry), but there I describe the real case that happened to
me, and the current behavior made it much harder to find out what was
happening.

That is the one:
https://lore.kernel.org/git/CACjc_5pdijCZrrXQWHswsxYrGUzZ7pZq+nj3SzY1z+Xxop64Ww@mail.gmail.com/T/#u

If the configuration file is there, but it behaves as it is not, one
will quickly point its fingers to the software that reads it.

In the end, if I had seen the message "Permission denied." that was
stored in "$!", I would have had a better clue that the problem was
with AppArmor, and not Gitweb.

There were a few unanswered questions in stack overflow regarding the
issue of enabling features on Gitweb when using "git instaweb" in a
persistent way, some of them might have been linked to that.

- gitweb refusing to blame:
https://serverfault.com/questions/551762/gitweb-refusing-to-blame
- How do I enable "blame" when using git instaweb?:
https://stackoverflow.com/questions/66688084/how-do-i-enable-blame-when-using-git-instaweb/77793405
- My own question: Problem with `git instaweb` on OpenSUSE Tumbleweed:
/etc/gitweb-common.conf is not being read:
https://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n

Let's see if someone comes up with a good reason to find the devil not guilty :)

Regards,
Marcelo.

On Wed, Jan 10, 2024 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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;
> >       }