threads / discuss / 60713

Problem in gitweb.cgi

Subject: Problem in gitweb.cgi

## tl;dr

5 messages between Jan 10, 2024 and Jan 11, 2024.

replies: 4people: 2as markdown or json

Marcelo Roberto Jimenez· Jan 10, 2024, 00:43 UTC · lore
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.

Marcelo Roberto Jimenez· Jan 10, 2024, 22:05 UTC · re: Marcelo Roberto Jimenez · lore

[PATCH] gitweb: Fixes error handling when reading configuration

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
Marcelo Roberto Jimenez· Jan 10, 2024, 22:57 UTC · re: Marcelo Roberto Jimenez · lore

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

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
Junio C Hamano· Jan 11, 2024, 00:17 UTC · re: Marcelo Roberto Jimenez · lore

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

Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> writes:
Show 9 quoted lines
> 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.
Show 24 quoted lines
> 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;
>  	}
Marcelo Roberto Jimenez· Jan 11, 2024, 02:14 UTC · re: Junio C Hamano · lore

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

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:
Show 53 quoted lines
>
> 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;
> >       }

← back to recent threads