Problem in gitweb.cgi

* Problem in gitweb.cgi
@ 2024-01-10  0:43 Marcelo Roberto Jimenez
  2024-01-10 22:05 ` [PATCH] gitweb: Fixes error handling when reading configuration Marcelo Roberto Jimenez
  2024-01-10 22:57 ` [PATCH v2] " Marcelo Roberto Jimenez
  0 siblings, 2 replies; 5+ messages in thread
From: Marcelo Roberto Jimenez @ 2024-01-10  0:43 UTC (permalink / raw)
  To: git

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread