gitweb problem?

gitweb problem?

[Eli Barzilay]

Whenever I view the toplevel gitweb page (running as a cgi script
under apache), but not when in a specific repo, I get this in my error
log:

gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.
fatal: error processing config file(s)
gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.
gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.

(taken verbatim from the apache error log, removed uninteresting line
prefixes.)

I'm using the pathinfo option, so perhaps there is a problem with that
setup?

Looking at the source, the last two line numbers are in
`git_get_project_config' -- so my guess is that the code is trying to
get the options from the repository config file even when showing the
toplevel page.  Based on this, and also guessing that $git_dir is
unset when viewing the toplevel page, I added

	return unless (defined $git_dir);

to the top (of the `git_get_project_config' function), and I get no
warnings and everything works as it should.

(Disclaimer: I can barely read perl, and I'm a git newbie, so all of
this can be due to some other stupid mistake.)

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: gitweb problem?

[Jakub Narebski]

Eli Barzilay <eli@barzilay.org> writes:

> Whenever I view the toplevel gitweb page (running as a cgi script
> under apache), but not when in a specific repo, I get this in my error
> log:
> 
> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.
> fatal: error processing config file(s)
> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.
> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.
> 
> (taken verbatim from the apache error log, removed uninteresting line
> prefixes.)

In the future (or if my diagnosis would turn out to be incorrect)
could you please show which lines are those (in *your* gitweb.cgi),
or at least provide gitweb version?  Changes to gitweb can render
such line numbers invalid.
 
> I'm using the pathinfo option, so perhaps there is a problem with that
> setup?

In the future (or if my diagnosis would turn out to be incorrect) could
you please include relevant parts of $GITWEB_CONFIG (by default it is
gitweb_config.perl), at least which features you have enabled, and how
they are configured?

> 
> Looking at the source, the last two line numbers are in
> `git_get_project_config' -- so my guess is that the code is trying to
> get the options from the repository config file even when showing the
> toplevel page.  Based on this, and also guessing that $git_dir is
> unset when viewing the toplevel page, I added
> 
> 	return unless (defined $git_dir);
> 
> to the top (of the `git_get_project_config' function), and I get no
> warnings and everything works as it should.
> 
> (Disclaimer: I can barely read perl, and I'm a git newbie, so all of
> this can be due to some other stupid mistake.)

That looks like lack of hardening against pilot error.  The
git_get_project_config should never be run when $git_dir is not set, as it
is meant to access *project* config.  

But at the top of git_project_list_body subroutine, which is responsible for
generating toplevel page with list of projects, we have:
	my $check_forks = gitweb_check_feature('forks');
and a bit later
	my $show_ctags = gitweb_check_feature('ctags');

Now both of those features are marked as not supporting project specific
override.  It might be that you by accident set $feature{XXX}{'override'}
to true... but I might be mistaken.


It would be strange that this bug was not detected by t9500 test...
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: gitweb problem?

[Eli Barzilay]

Jakub Narebski <jnareb@gmail.com> writes:

> Eli Barzilay <eli@barzilay.org> writes:
>
>> Whenever I view the toplevel gitweb page (running as a cgi script
>> under apache), but not when in a specific repo, I get this in my error
>> log:
>> 
>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.
>> fatal: error processing config file(s)
>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.
>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.
>> 
>> (taken verbatim from the apache error log, removed uninteresting line
>> prefixes.)
>
> In the future (or if my diagnosis would turn out to be incorrect)
> could you please show which lines are those (in *your* gitweb.cgi),
> or at least provide gitweb version?  Changes to gitweb can render
> such line numbers invalid.

Sorry, I should have mentioned that this is from 1.7.0 -- my file is
not modified, and built with no changes to the defaults except for
prefix=/usr/local.  In any case, the script -- with the test that I
added -- is at http://tmp.barzilay.org/x1 .


>> I'm using the pathinfo option, so perhaps there is a problem with
>> that setup?
>
> In the future (or if my diagnosis would turn out to be incorrect)
> could you please include relevant parts of $GITWEB_CONFIG (by
> default it is gitweb_config.perl), at least which features you have
> enabled, and how they are configured?

http://tmp.barzilay.org/x2 (but see below).


>> Looking at the source, the last two line numbers are in
>> `git_get_project_config' -- so my guess is that the code is trying to
>> get the options from the repository config file even when showing the
>> toplevel page.  Based on this, and also guessing that $git_dir is
>> unset when viewing the toplevel page, I added
>> 
>> 	return unless (defined $git_dir);
>> 
>> to the top (of the `git_get_project_config' function), and I get no
>> warnings and everything works as it should.
>> 
>> (Disclaimer: I can barely read perl, and I'm a git newbie, so all of
>> this can be due to some other stupid mistake.)
>
> That looks like lack of hardening against pilot error.  The
> git_get_project_config should never be run when $git_dir is not set,
> as it is meant to access *project* config.
>
> But at the top of git_project_list_body subroutine, which is
> responsible for generating toplevel page with list of projects, we
> have:
> 	my $check_forks = gitweb_check_feature('forks');
> and a bit later
> 	my $show_ctags = gitweb_check_feature('ctags');
>
> Now both of those features are marked as not supporting project specific
> override.  It might be that you by accident set $feature{XXX}{'override'}
> to true... but I might be mistaken.

No, that's not it.  I've tracked all calls to
`git_get_project_config', and the two offending calls are the ones in
`feature_snapshot' and `feature_avatar'.  I then verified that
commenting out these two lines in my config

  $feature{'snapshot'}{'override'} = 1;
  $feature{'avatar'}{'override'} = 1;

avoids the calls -- and the comments indicate that it should be fine
to do that.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: gitweb problem?

[Jakub Narebski]

Eli Barzilay <eli@barzilay.org> writes:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Eli Barzilay <eli@barzilay.org> writes:
>>
>>> Whenever I view the toplevel gitweb page (running as a cgi script
>>> under apache), but not when in a specific repo, I get this in my error
>>> log:
>>> 
>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2065.
>>> fatal: error processing config file(s)
>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2221.
>>> gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at /home/git/gitweb/gitweb.cgi line 2218.
[...]
>>> Looking at the source, the last two line numbers are in
>>> `git_get_project_config' -- so my guess is that the code is trying to
>>> get the options from the repository config file even when showing the
>>> toplevel page.  Based on this, and also guessing that $git_dir is
>>> unset when viewing the toplevel page, I added
>>> 
>>> 	return unless (defined $git_dir);
>>> 
>>> to the top (of the `git_get_project_config' function), and I get no
>>> warnings and everything works as it should.

BTW. the first line number is in git_cmd invoked (called) from
git_get_project_config.

[...]
> No, that's not it.  I've tracked all calls to
> `git_get_project_config', and the two offending calls are the ones in
> `feature_snapshot' and `feature_avatar'.  I then verified that
> commenting out these two lines in my config
> 
>   $feature{'snapshot'}{'override'} = 1;
>   $feature{'avatar'}{'override'} = 1;
> 
> avoids the calls -- and the comments indicate that it should be fine
> to do that.

Ah, now I have found it.  This is a bug in gitweb.

The problem is in the following fragment:

  # path to the current git repository
  our $git_dir;
  $git_dir = "$projectroot/$project" if $project;

  # list of supported snapshot formats
  our @snapshot_fmts = gitweb_get_feature('snapshot');
  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);

For the toplevel gitweb page, which is the list of projects, $project
is not defined, therefore $git_dir is neither.  gitweb_get_feature()
subroutine calls git_get_project_config() if project specific override
is turned on... but we don't have project here.

There are (at least) three possible solutions:

1. Harden gitweb_get_feature() so that it doesn't call
   git_get_project_config() if $project (and therefore $git_dir) is not
   defined; there is no project for project specific config.

2. Harden git_get_project_config() like you did in your fix, returning
   early if $git_dir is not defined.

3. Harden git_cmd() so that it doesn't add "--git-dir=$git_dir" if
   $git_dir is not defined, and change git_get_project_config() so that
   it doesn't even try to access $git_dir if it is not defined.

   	# get config
   	if (!defined $config_file ||
   	    $config_file ne "$git_dir/config") {
   		%config = git_parse_project_config('gitweb');
   		$config_file = "$git_dir/config";
   	}


The last solution, while requiring most work, has the advantage of being
able to do override via user (~apache/.gitconfig) or system (/etc/gitconfig)
gitweb config file.  But I am not sure if it is worth it...


As to why t9500 test didn't detect that: there is no test for project list
page when features are configured to be overridable...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

[PATCH] gitweb: Fix project-specific feature override behavior

[Jakub Narebski]

This commit fixes a bug in processing project-specific override in
a situation when there is no project, e.g. for the projects list page.

When 'snapshot' feature had project specific config override enabled
by putting
  $feature{'snapshot'}{'override'} = 1;

(or equivalent) in $GITWEB_CONFIG, and when viewing toplevel gitweb
page, which means the projects list page (to be more exact this
happens for any project-less action), gitweb would put the following
Perl warnings in error log:

  gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at gitweb.cgi line 2065.
  fatal: error processing config file(s)
  gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at gitweb.cgi line 2221.
  gitweb.cgi: Use of uninitialized value $git_dir in concatenation (.) or string at gitweb.cgi line 2218.


The problem is in the following fragment of code:

  # path to the current git repository
  our $git_dir;
  $git_dir = "$projectroot/$project" if $project;

  # list of supported snapshot formats
  our @snapshot_fmts = gitweb_get_feature('snapshot');
  @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);

For the toplevel gitweb page, which is the list of projects, $project is not
defined, therefore neither is $git_dir.  gitweb_get_feature() subroutine
calls git_get_project_config() if project specific override is turned
on... but we don't have project here.


Those errors mentioned above occur in the following fragment of code in
git_get_project_config():

  	# get config
  	if (!defined $config_file ||
  	    $config_file ne "$git_dir/config") {
  		%config = git_parse_project_config('gitweb');
  		$config_file = "$git_dir/config";
  	}

git_parse_project_config() calls git_cmd() which has '--git-dir='.$git_dir


There are (at least) three possible solutions:
1. Harden gitweb_get_feature() so that it doesn't call
   git_get_project_config() if $project (and therefore $git_dir) is not
   defined; there is no project for project specific config.
2. Harden git_get_project_config() like you did in your fix, returning early
   if $git_dir is not defined.
3. Harden git_cmd() so that it doesn't add "--git-dir=$git_dir" if $git_dir
   is not defined, and change git_get_project_config() so that it doesn't
   even try to access $git_dir if it is not defined.

This commit implements both 1.) and 2.), i.e. gitweb_get_feature() doesn't
call project-specific override if $git_dir is not defined (if there is no
project), and git_get_project_config() returns early if $git_dir is not
defined.


Add a test for this bug to t/t9500-gitweb-standalone-no-errors.sh test.

Reported-by: Eli Barzilay <eli@barzilay.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is a fix, with much too long commit message ;-)

 gitweb/gitweb.perl                     |    9 ++++++++-
 t/t9500-gitweb-standalone-no-errors.sh |   16 ++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1ce4de1..d02734e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -467,7 +467,11 @@ sub gitweb_get_feature {
 		$feature{$name}{'sub'},
 		$feature{$name}{'override'},
 		@{$feature{$name}{'default'}});
-	if (!$override) { return @defaults; }
+	# project specific override is possible only if we have project
+	our $git_dir; # global variable, declared later
+	if (!$override || !defined $git_dir) {
+		return @defaults;
+	}
 	if (!defined $sub) {
 		warn "feature $name is not overridable";
 		return @defaults;
@@ -2225,6 +2229,9 @@ sub config_to_multi {
 sub git_get_project_config {
 	my ($key, $type) = @_;
 
+	# do we have project
+	return unless (defined $project && defined $git_dir);
+
 	# key sanity check
 	return unless ($key);
 	$key =~ s/^gitweb\.//;
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 0fe9beb..4f2b9b0 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -591,14 +591,22 @@ test_debug 'cat gitweb.log'
 # ----------------------------------------------------------------------
 # gitweb config and repo config
 
-cat >>gitweb_config.perl <<EOF
+cat >>gitweb_config.perl <<\EOF
 
-\$feature{'blame'}{'override'} = 1;
-\$feature{'snapshot'}{'override'} = 1;
-\$feature{'avatar'}{'override'} = 1;
+# turn on override for each overridable feature
+foreach my $key (keys %feature) {
+	if ($feature{$key}{'sub'}) {
+		$feature{$key}{'override'} = 1;
+	}
+}
 EOF
 
 test_expect_success \
+	'config override: projects list (implicit)' \
+	'gitweb_run'
+test_debug 'cat gitweb.log'
+
+test_expect_success \
 	'config override: tree view, features not overridden in repo config' \
 	'gitweb_run "p=.git;a=tree"'
 test_debug 'cat gitweb.log'

Re: gitweb problem?

[Eli Barzilay]

Jakub Narebski <jnareb@gmail.com> writes:

> BTW. the first line number is in git_cmd invoked (called) from
> git_get_project_config.

(Oh yeah, I forgot about that one, sorry.)

> [...]
>
> The last solution, while requiring most work, has the advantage of
> being able to do override via user (~apache/.gitconfig) or system
> (/etc/gitconfig) gitweb config file.  But I am not sure if it is
> worth it...

Thanks for the fix!  As for the point above, isn't the whole point of
the feature system to do this kind of toplevel configuration possible?
(If so, it sounds like a concrete explanation for why it's not worth
the effort: it only makes it possible to specify the defaults in a
toplevel .gitconfig file, which could be nice but probably confusing.)

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!