Potential bug in gitweb 1.7.2

DISCUSSION Julio Lajara 5 participants 15 messages Jun 15, 2016

Share & tools

Julio Lajara

author

2016-06-15 22:49 UTC #0 lore raw

Hi all, I recently had git/gitweb (1.7.0.4) setup on my Ubuntu 10.04 box and decided to add ppa:git-core/ppa to get up to 1.7.2. After I did so, my previously working gitweb stopped functioning and only shows a blank page. Im using apache/cgi and the following error is what I found in my apache error log:

[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call method "http" on an undefined value at /usr/share/gitweb/index.cgi line 3401., referer: http://192.168.1.142/gitweb/

From the latest source:
cat -n gitweb.perl | grep 3401
3401 if (defined $cgi->http('HTTP_ACCEPT') &&

Thanks,

Jonathan Nieder

2016-06-15 22:49 UTC #1 lore raw

Hi Julio,

Quoted 3 lines · Julio Lajara

Hi all, I recently had git/gitweb (1.7.0.4) setup on my Ubuntu 10.04 box and decided to add ppa:git-core/ppa to get up to 1.7.2. After I did so, my previously working gitweb stopped functioning

Indeed, the rules for installation have changed with v1.7.2, so the packaging may need to be adjusted[1] to match (see gitweb/INSTALL). Please report this to the Ubuntu git team.

(I am not sure what the proper way to do that is.
https://bugs.launchpad.net/ubuntu/+source/git/+filebug will at least reach the right people.)

Hope that helps,
Jonathan

[1] e.g. http://repo.or.cz/w/debian-git/jrn.git/commit/ff41c31c5

Anders Kaseorg

2016-06-15 22:49 UTC #2 lore raw

Quoted 5 lines · Jonathan Nieder

Indeed, the rules for installation have changed with v1.7.2, so the packaging may need to be adjusted[1] to match (see gitweb/INSTALL). Please report this to the Ubuntu git team.
[1] e.g. http://repo.or.cz/w/debian-git/jrn.git/commit/ff41c31c5

Thanks for the report. But, the 1.7.2-0ppa1 package in the git-core PPA was constructed by merging your jrn/for-gerrit branch (1.7.2~rc3-0.1, commit f84e1eb) with upstream v1.7.2, and updating debian/versions.upstream and debian/changelog.upstream. It already includes that commit.

Indeed, when I install the gitweb 1.7.2-0ppa1 package, put some repositories into /var/cache/git, and go to http://localhost/gitweb/ , it works correctly for me.

The error that Julio originally posted:

Quoted 3 lines

Quoted 3 lines
[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call 
method "http" on an undefined value at /usr/share/gitweb/index.cgi line 3401., referer: http://192.168.1.142/gitweb/

looks very strange, and does not seem like it would result from just a packaging problem.

Anders

Jonathan Nieder

2016-06-15 22:49 UTC #3 lore raw

Hi Anders,

Quoted 5 lines · Anders Kaseorg

the 1.7.2-0ppa1 package in the git-core PPA
was constructed by merging your jrn/for-gerrit branch (1.7.2~rc3-0.1, commit f84e1eb) with upstream v1.7.2, and updating debian/versions.upstream and debian/changelog.upstream. It already includes that commit.

Good to know. Is there a public git repository for the PPA? (I am asking selfishly, because it would be convenient to be able to tell what commit each version corresponds to and to steal changes back.)

Quoted 3 lines

Indeed, when I install the gitweb 1.7.2-0ppa1 package, put some repositories into /var/cache/git, and go to http://localhost/gitweb/ , it works correctly for me.

Hm, I should have tried that myself.

Thanks.

Anders Kaseorg

2016-06-15 22:49 UTC #4 lore raw

Quoted 3 lines · Jonathan Nieder

Good to know. Is there a public git repository for the PPA? (I am asking selfishly, because it would be convenient to be able to tell what commit each version corresponds to and to steal changes back.)

There is now:
git://andersk.mit.edu/git.git
http://andersk.mit.edu/gitweb/git.git
(The most recent version is on the ppa branch, but there are tags going back through 1.7.0.1-1~ppa1.)

Anders

Jonathan Nieder

2016-06-15 22:49 UTC #5 lore raw

Quoted 1 line · Anders Kaseorg

git://andersk.mit.edu/git.git

Thanks!

FWIW (because this does not seem to have been advertised widely) there's a mailing list to coordinate work on the Debian git packages.

Anyone interested can send mail (with any content) to <[email protected]> for a list of supported commands or <[email protected]> to subscribe.

Julio Lajara

author

2016-06-15 22:49 UTC #6 lore raw

In reply to Anders Kaseorg

Hi Anders, thanks for the input. I will begin chasing down the issue in my config then, but its strange that it would stop working as the exact same configuration works without issue in 1.7.0.4 . Ill follow up with what I find.

Thanks,

Quoted 31 lines · Anders Kaseorg

Quoted 6 lines · Jonathan Nieder
Indeed, the rules for installation have changed with v1.7.2, so the packaging may need to be adjusted[1] to match (see gitweb/INSTALL). Please report this to the Ubuntu git team.
[1] e.g. http://repo.or.cz/w/debian-git/jrn.git/commit/ff41c31c5
Thanks for the report. But, the 1.7.2-0ppa1 package in the git-core PPA was constructed by merging your jrn/for-gerrit branch (1.7.2~rc3-0.1, commit f84e1eb) with upstream v1.7.2, and updating debian/versions.upstream and debian/changelog.upstream. It already includes that commit.
Indeed, when I install the gitweb 1.7.2-0ppa1 package, put some repositories into /var/cache/git, and go to http://localhost/gitweb/ , it works correctly for me.
The error that Julio originally posted:
Quoted 4 lines
Quoted 4 lines
[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
method "http" on an undefined value at /usr/share/gitweb/index.cgi line 3401., referer: http://192.168.1.142/gitweb/
looks very strange, and does not seem like it would result from just a packaging problem.
Anders

Jakub Narebski

2016-06-15 22:49 UTC #7 lore raw

Quoted 5 lines · Julio Lajara

Hi Anders, thanks for the input. I will begin chasing down the issue in my config then, but its strange that it would stop working as the exact same configuration works without issue in 1.7.0.4 . Ill follow up with what I find.

Quoted 11 lines

Quoted 9 lines · Anders Kaseorg
The error that Julio originally posted:
Quoted 3 lines
Quoted 3 lines
[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
method "http" on an undefined value at /usr/share/gitweb/index.cgi line 3401., referer: http://192.168.1.142/gitweb/
looks very strange, and does not seem like it would result from just a packaging problem.

Julio, could you show us the neigbourhood of line 3401 in your index.cgi file? What is this index.cgi file - git doesn't have it?

Signature · 2 lines

Jakub Narebski
Poland

Ævar Arnfjörð Bjarmason

2016-06-15 22:49 UTC #8 lore raw

Quoted 16 lines · Jakub Narebski

Quoted 14 lines · Julio Lajara
Hi Anders, thanks for the input. I will begin chasing down the issue in my config then, but its strange that it would stop working as the exact same configuration works without issue in 1.7.0.4 . Ill follow up with what I find.
Quoted 7 lines · Anders Kaseorg
The error that Julio originally posted:
Quoted 3 lines
Quoted 3 lines
[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
method "http" on an undefined value at /usr/share/gitweb/index.cgi line 3401., referer: http://192.168.1.142/gitweb/

Quoted 5 lines

Quoted 2 lines
Quoted 2 lines
looks very strange, and does not seem like it would result from just a packaging problem.
Julio, could you show us the neigbourhood of line 3401 in your index.cgi file? What is this index.cgi file - git doesn't have it?

His index.cgi is just gitweb.cgi under a different name. Line 3401 is the same as gitweb/gitweb.cgi in the 1.7.2 release.

Julio Lajara

author

2016-06-15 22:49 UTC #9 lore raw

Hi, Ive managed to narrow down the issue to a line in the gitolite's contrib/gitweb/gitweb.conf that I appended to the default /etc/gitweb.conf . It was working with 1.7.0.4 but 1.7.2 doesnt like this line:

$ENV{GL_USER} = $cgi->remote_user || "gitweb";

I corrected it by just setting it equal to gitweb and all is well now.

Thanks,

Quoted 30 lines · Ævar Arnfjörð Bjarmason

Quoted 19 lines · Jakub Narebski
Quoted 16 lines · Julio Lajara
Hi Anders, thanks for the input. I will begin chasing down the issue in my config then, but its strange that it would stop working as the exact same configuration works without issue in 1.7.0.4 . Ill follow up with what I find.
Quoted 8 lines · Anders Kaseorg
The error that Julio originally posted:
Quoted 4 lines
Quoted 4 lines
[Sat Jul 24 23:11:41 2010] [error] [client 192.168.1.100] Can't call
method "http" on an undefined value at /usr/share/gitweb/index.cgi line 3401., referer: http://192.168.1.142/gitweb/
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
looks very strange, and does not seem like it would result from just a packaging problem.
Julio, could you show us the neigbourhood of line 3401 in your index.cgi file? What is this index.cgi file - git doesn't have it?
His index.cgi is just gitweb.cgi under a different name. Line 3401 is the same as gitweb/gitweb.cgi in the 1.7.2 release.

Jakub Narebski

2016-06-15 22:49 UTC #10 lore raw

Quoted 8 lines · Julio Lajara

Hi, Ive managed to narrow down the issue to a line in the gitolite's contrib/gitweb/gitweb.conf that I appended to the default /etc/gitweb.conf . It was working with 1.7.0.4 but 1.7.2 doesnt like this line:
$ENV{GL_USER} = $cgi->remote_user || "gitweb";
I corrected it by just setting it equal to gitweb and all is well now.

I think the following commit is the reason that this stopped working 869d588 (gitweb: Move evaluate_gitweb_config out of run_request, 2010-07-05)

Quoted 1 line

From its commit message:

gitweb: Move evaluate_gitweb_config out of run_request

Move evaluate_gitweb_config() and evaluate_git_version() out of run_request() to run(), making them not run one for each request. This changes how git behaves in FastCGI case.

This change makes it impossible to have config which changes with request, but I don't think anyone relied on such (hidden action) behavior.

I guess I was wrong about that. I wonder if it would be possible to re-enable this feature (which I think is needed to be able to use $cgi->remote_user) but without having all pay the [slight] performance penalty of including (and I think parsing) config file once per each request.

Signature · 2 lines

Jakub Narebski
Poland

Jonathan Nieder

2016-06-15 22:49 UTC #11 lore raw

[PATCH/RFC] gitweb: allow configurations that change with each request

gitolite's contrib/gitweb/gitweb.conf includes:

	$ENV{GL_USER} = $cgi->remote_user || "gitweb";

which is useful for setups where a user has to be authenticated to access certain repos. Perhaps other typical configurations change per session in other ways, too.

v1.7.2-rc2~6 (gitweb: Move evaluate_gitweb_config out of run_request, 2010-07-05) broke such configurations for a speedup, by loading the configuration once per FastCGI process.

Probably in the end there should be a way to specify in the configuration whether a particular installation wants the speedup or the flexibility. But for now it is easier to just undo the relevant change.

This partially reverts commit 869d58813b24c74e84c9388041eafcef40cb51e4.

Reported-by Julio Lajara <[email protected]>

Analysis-by Jakub Narebski <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---

Quoted 5 lines · Jakub Narebski

I wonder if it would be possible to
re-enable this feature (which I think is needed to be able to use $cgi->remote_user) but without having all pay the [slight] performance penalty of including (and I think parsing) config file once per each request.

I dunno. Maybe this would be a good place to start.

 gitweb/gitweb.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

gitweb/gitweb.perl +4 -4

index e0e9532..300c4b1 100755
@@ -1060,8 +1060,12 @@ sub run_request {
 	reset_timer();
 
 	evaluate_uri();
+	evaluate_gitweb_config();
 	check_loadavg();
 
+	# $projectroot and $projects_list might be set in gitweb config file
+	$projects_list ||= $projectroot;
+
 	evaluate_query_params();
 	evaluate_path_info();
 	evaluate_and_validate_params();

@@ -1109,12 +1113,8 @@ sub evaluate_argv {
 
 sub run {
 	evaluate_argv();
-	evaluate_gitweb_config();
 	evaluate_git_version();
 
-	# $projectroot and $projects_list might be set in gitweb config file
-	$projects_list ||= $projectroot;
-
 	$pre_listen_hook->()
 		if $pre_listen_hook;

Signature · 1 line

1.7.2.1.544.ga752d.dirty

Jakub Narebski

2016-06-15 22:49 UTC #12 lore raw

Re: [PATCH/RFC] gitweb: allow configurations that change with each request

Quoted 12 lines · Jonathan Nieder

gitolite's contrib/gitweb/gitweb.conf includes:
	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
which is useful for setups where a user has to be authenticated to access certain repos. Perhaps other typical configurations change per session in other ways, too.
v1.7.2-rc2~6 (gitweb: Move evaluate_gitweb_config out of run_request, 2010-07-05) broke such configurations for a speedup, by loading the configuration once per FastCGI process.

Quoted 6 lines

Probably in the end there should be a way to specify in the configuration whether a particular installation wants the speedup or the flexibility. But for now it is easier to just undo the relevant change.
This partially reverts commit 869d58813b24c74e84c9388041eafcef40cb51e4.

Why only *partially* reverts...

...ah, I see, I did "while at it" change fixing timer and number of git commands info for persistent environments (i.e. gitweb run as FastCGI script).

Quoted 3 lines

Reported-by Julio Lajara <[email protected]>
Analysis-by Jakub Narebski <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

Reluctantly-acked-by Jakub Narebski <[email protected]>

Quoted 10 lines

---
Quoted 5 lines · Jakub Narebski
I wonder if it would be possible to
re-enable this feature (which I think is needed to be able to use $cgi->remote_user) but without having all pay the [slight] performance penalty of including (and I think parsing) config file once per each request.
I dunno. Maybe this would be a good place to start.

Currently this is non-issue; the eventual performance penalty is I guess very small.

The problem would be with adding caching support to gitweb. While default GitwebCache::* caching engine should have low startup cost, the CHI generic caching interface that one might want to use instead uses Moose (Mouse with Any::Moose?) for OOP, which imposes some startup cost. One enables and configures output caching in gitweb config, so if gitweb config would be read once per run then cache interface could be started once per run, not once per request.

Nevertheless this change is backwards incompatibile change, and should probably wait for 1.7.3 (pity that 1.7.2 was so recently released).

One solution I can think of (still backwards incompatibile) would be to provide $per_request_config variable, which would hold anonymous sub with parts of config that need to be done per request (this should work with global variables (our), but I think it wouldn't work with lexical variables (my)). For example gitolite's contrib/gitweb/gitweb.conf would then include:

  $per_request_config = sub {
  	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
  }

What do you think about it?

Quoted 23 lines

 gitweb/gitweb.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

gitweb/gitweb.perl +4 -0

index e0e9532..300c4b1 100755
@@ -1060,8 +1060,12 @@ sub run_request {
 	reset_timer();
 
 	evaluate_uri();
+	evaluate_gitweb_config();
 	check_loadavg();
 
+	# $projectroot and $projects_list might be set in gitweb config file
+	$projects_list ||= $projectroot;
+
 	evaluate_query_params();
 	evaluate_path_info();
 	evaluate_and_validate_params();

@@ -1109,12 +1113,8 @@ sub evaluate_argv {
 
 sub run {

gitweb/gitweb.perl sub evaluate_argv { · line 1120

Quoted 10 lines

gitweb/gitweb.perl +0 -4

 	evaluate_argv();
-	evaluate_gitweb_config();
 	evaluate_git_version();
 
-	# $projectroot and $projects_list might be set in gitweb config file
-	$projects_list ||= $projectroot;
-
 	$pre_listen_hook->()
 		if $pre_listen_hook;

That reminds me: '$projects_list ||= $projectroot;' line should be put, I think, inside evaluate_gitweb_config(). But that of course should not be done in this patch.

Signature · 2 lines

Jakub Narebski
Poland

Jonathan Nieder

2016-06-15 22:49 UTC #13 lore raw

Re: [PATCH/RFC] gitweb: allow configurations that change with each request

Quoted 10 lines · Jakub Narebski

One solution I can think of (still backwards incompatibile) would be to provide $per_request_config variable, which would hold anonymous sub with parts of config that need to be done per request (this should work with global variables (our), but I think it wouldn't work with lexical variables (my)). For example gitolite's contrib/gitweb/gitweb.conf would then include:
  $per_request_config = sub {
  	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
  }

How would that interact with caching? i.e., how would whatever caching engine is used learn that $cgi->remote_user is part of the key needed to uniquely determine a response?

Jakub Narebski

2016-06-15 22:49 UTC #14 lore raw

Re: [PATCH/RFC] gitweb: allow configurations that change with each request

Quoted 9 lines · Jonathan Nieder

Quoted 7 lines · Jakub Narebski
One solution I can think of (still backwards incompatibile) would be to provide $per_request_config variable, which would hold anonymous sub with parts of config that need to be done per request (this should work with global variables (our), but I think it wouldn't work with lexical variables (my)). For example gitolite's contrib/gitweb/gitweb.conf would then include:

Quoted 7 lines

Quoted 3 lines
  $per_request_config = sub {
  	$ENV{GL_USER} = $cgi->remote_user || "gitweb";
  }
How would that interact with caching? i.e., how would whatever caching engine is used learn that $cgi->remote_user is part of the key needed to uniquely determine a response?

Well, obviously caching itself, including generating cache key, would be done per-request. It is only *initializing* cache (i.e. require +
+ $cache->new()) that should be done once per run and not once per request.

But nevermind, this might be not much of a problem. I'll start with cache re-initialized on each request for start.

Signature · 2 lines

Jakub Narebski
Poland