Fwd: Git-web error

Fwd: Git-web error

[rajesh boyapati]

---------- Forwarded message ----------
From: rajesh boyapati <boyapatisraj...@gmail.com>
Date: Jan 25, 7:10 pm
Subject: Git-web error
To: Repo and Gerrit Discussion


Hi,

We integrated git-web to our gerrit Code-review.
I installed gitweb using the command:
$ sudo apt-get install gitweb
Then I configured gitweb to our gerrit using the command:
$ git config --file $SITE_PATH/etc/gerrit.config gitweb.cgi /usr/lib/
cgi-bin/gitweb.cgi
Now the gitweb is added to gerrit.config and in gerrit config file, it
looks like
[gitweb]
        cgi = /usr/lib/cgi-bin/gitweb.cgi
Then, restarted gerrit server.

When I go to one of the projects in gerrit through gitweb and when I
click "summary", I am getting the below error.
If I click other tabs(log, shortlog, commit, tree,etc) after clicking
"summary", I am getting following error in error-log.
If I click other tabs(log, shortlog, commit, tree,etc) before clicking
"summary", everything works fine.

Error:
=================================================================
[2012-01-25 18:50:32,334] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
18:50:32 2012] gitweb.cgi: Use of uninitialized value $head in string
eq at /usr/lib/cgi-bin/gitweb.cgi line 4720.
[2012-01-25 18:50:35,658] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
18:50:35 2012] gitweb.cgi: Use of uninitialized value $commit_id in
open at /usr/lib/cgi-bin/gitweb.cgi line 2817.
[2012-01-25 18:50:35,660] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad
revision ''
[2012-01-25 18:50:39,209] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
18:50:39 2012] gitweb.cgi: Use of uninitialized value $commit_id in
open at /usr/lib/cgi-bin/gitweb.cgi line 2817.
[2012-01-25 18:50:39,210] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad
revision ''
[2012-01-25 18:50:40,656] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad
revision 'HEAD'
[2012-01-25 18:53:17,097] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad
revision 'HEAD'
[2012-01-25 18:53:17,113] ERROR
com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
18:53:17 2012] gitweb.cgi: Use of uninitialized value $head in string
eq at /usr/lib/cgi-bin/gitweb.cgi line 4720.
=================================================================

Can any one help me on how to resolve this issue?.

Thanks,
Rajesh.

Re: Fwd: Gitweb error

[Jakub Narebski]

rajesh boyapati <boyapatisrajesh@gmail.com> writes:

> We integrated git-web to our gerrit Code-review.
> I installed gitweb using the command:
> $ sudo apt-get install gitweb
> Then I configured gitweb to our gerrit using the command:
> $ git config --file $SITE_PATH/etc/gerrit.config gitweb.cgi /usr/lib/
> cgi-bin/gitweb.cgi
> Now the gitweb is added to gerrit.config and in gerrit config file, it
> looks like
> [gitweb]
>         cgi = /usr/lib/cgi-bin/gitweb.cgi
> Then, restarted gerrit server.

This combination of gitweb (which version?) and Gerrit can be hard to
debug, unfortunately.
 
> When I go to one of the projects in gerrit through gitweb and when I
> click "summary", I am getting the below error.
> If I click other tabs(log, shortlog, commit, tree,etc) after clicking
> "summary", I am getting following error in error-log.
> If I click other tabs(log, shortlog, commit, tree,etc) before clicking
> "summary", everything works fine.
> 
> Error:
> =================================================================
> [2012-01-25 18:50:32,334] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
> 18:50:32 2012] gitweb.cgi: Use of uninitialized value $head in string
> eq at /usr/lib/cgi-bin/gitweb.cgi line 4720.

Could you show this line and about 3 lines of context in your
gitweb.cgi?

> [2012-01-25 18:50:35,658] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
> 18:50:35 2012] gitweb.cgi: Use of uninitialized value $commit_id in
> open at /usr/lib/cgi-bin/gitweb.cgi line 2817.

Same here.

> [2012-01-25 18:50:35,660] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad
> revision ''

It looks like gitweb incorrectly gets empty revision
parameter... strange.


-- 
Jakub Narebski

Re: Fwd: Gitweb error

[Jakub Narebski]

On Fri, 27 Jan 2012, rajesh boyapati wrote:
> On Fri, Jan 27, 2012 at 3:39 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> > rajesh boyapati <boyapatisrajesh@gmail.com> writes:

> > > When I go to one of the projects in gerrit through gitweb and when I
> > > click "summary", I am getting the below error.
> > > If I click other tabs(log, shortlog, commit, tree,etc) after clicking
> > > "summary", I am getting following error in error-log.
> > > If I click other tabs(log, shortlog, commit, tree,etc) before clicking
> > > "summary", everything works fine.

It is strange that you get an intermittent error like this, and make
it even harder to debug - HTTP is stateless.

> > > Error:
> > > =================================================================
> > > [2012-01-25 18:50:32,334] ERROR
> > > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
> > > 18:50:32 2012] gitweb.cgi: Use of uninitialized value $head in string
> > > eq at /usr/lib/cgi-bin/gitweb.cgi line 4720.
> >
> > Could you show this line and about 3 lines of context in your
> > gitweb.cgi?
> >
> 
>     my $alternate = 1;
>     for (my $i = $from; $i <= $to; $i++) {
>         my $entry = $headlist->[$i];
>         my %ref = %$entry;
>         my $curr = $ref{'id'} eq $head;
>         if ($alternate) {
>             print "<tr class=\"dark\">\n";
>         } else {
>             print "<tr class=\"light\">\n";
>         }
>         $alternate ^= 1;
 
Hmmmm... I see that we do not check if $head is defined here before using
it.  This can happen legitimately if we are on yet to be born orphan branch
(so $head, which should be named $head_at, is undefined) but there exist
other branches (so $headlist is not empty).

But I don't think it is what happened in your case, is it?

[...]
-- 
Jakub Narebski
Poland

Re: Fwd: Gitweb error

[Jakub Narebski]

On Mon, 30 Jan 2012, rajesh boyapati wrote:
> 2012/1/28 Jakub Narebski <jnareb@gmail.com>
>> On Fri, 27 Jan 2012, rajesh boyapati wrote:

>>>     my $alternate = 1;
>>>     for (my $i = $from; $i <= $to; $i++) {
>>>         my $entry = $headlist->[$i];
>>>         my %ref = %$entry;
>>>         my $curr = $ref{'id'} eq $head;
>>>         if ($alternate) {
>>>             print "<tr class=\"dark\">\n";
>>>         } else {
>>>             print "<tr class=\"light\">\n";
>>>         }
>>>         $alternate ^= 1;
>>
>> Hmmmm... I see that we do not check if $head is defined here before using
>> it.  This can happen legitimately if we are on yet to be born orphan branch
>> (so $head, which should be named $head_at, is undefined) but there exist
>> other branches (so $headlist is not empty).
>>
>> But I don't think it is what happened in your case, is it?

tldr; It did happen.
 
> For my git projects on gerrit, our main branch name is "base".
> We don't have any code on "master" branch.
> May be the $HEAD is looking for master branch(or checked out branch in git
> project).--> In our case, "master" is an empty branch.
> Also, In the git projects, the HEAD file is pointing to "ref:
> refs/heads/master".
> So, I think that's the reason for errors.
> 
> How can I make $HEAD to point to a branch other than "master"?.
>   a) I can do this by pointing HEAD file in git projects to other branch
[...]
>   b) Is there any way, other than doing above step (a) ?.
>      I mean I don't want to have a code on "master" branch and also I
>      don't want to point HEAD file in git projects to some other branch.
>      Do I need to make any modifications to "gitweb.cgi" for this?

Now that I know the source of this error, I can write test case
for it, and fix it.  I'll try to do it soon.

So finally what you would need for (b) is just upgrade gitweb.
-- 
Jakub Narebski
Poland

[PATCH] gitweb: Deal with HEAD pointing to unborn branch in "heads" view

[Jakub Narebski]

On Mon, 30 Jan 2012, rajesh boyapati wrote:
> 
> Thank you for your Suggestions in finding the error in my case.
> Do you have a blog, so that, I can know your updates about gitweb.

No, I don't have a blog, though I might use Git page on Google+ to
announce larger changes.  

I am posting this to you to announce partial fix for your problem
in the form of patch to gitweb, which would hopefully made it to
next git version.

> 2012/1/30 Jakub Narebski <jnareb@gmail.com>
>> On Mon, 30 Jan 2012, rajesh boyapati wrote:
>>> 2012/1/28 Jakub Narebski <jnareb@gmail.com>
>>>> On Fri, 27 Jan 2012, rajesh boyapati wrote:

>>>>>     my $alternate = 1;
>>>>>     for (my $i = $from; $i <= $to; $i++) {
>>>>>         my $entry = $headlist->[$i];
>>>>>         my %ref = %$entry;
>>>>>         my $curr = $ref{'id'} eq $head;
>>>>>         if ($alternate) {
>>>>>             print "<tr class=\"dark\">\n";
>>>>>         } else {
>>>>>             print "<tr class=\"light\">\n";
>>>>>         }
>>>>>         $alternate ^= 1;
>>>>
>>>> Hmmmm... I see that we do not check if $head is defined here before using
>>>> it.  This can happen legitimately if we are on yet to be born orphan branch
>>>> (so $head, which should be named $head_at, is undefined) but there exist
>>>> other branches (so $headlist is not empty).

This was simple to fix.

>>>>>>> [2012-01-25 18:50:35,658] ERROR
>>>>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
>>>>>>> 18:50:35 2012] gitweb.cgi: Use of uninitialized value $commit_id
>>>>>>> in open at /usr/lib/cgi-bin/gitweb.cgi line 2817.

>>>>> sub parse_commits {
>>>>>     my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
>>>>>     my @cos;
>>>>>
>>>>>     $maxcount ||= 1;
>>>>>     $skip ||= 0;
>>>>>
>>>>>     local $/ = "\0";
>>>>>
>>>>>     open my $fd, "-|", git_cmd(), "rev-list",
>>>>>         "--header",
>>>>>         @args,
>>>>>         ("--max-count=" . $maxcount),
>>>>>         ("--skip=" . $skip),
>>>>>         @extra_options,
>>>>>         $commit_id,
>>>>>         "--",
>>>>>         ($filename ? ($filename) : ())
>>>>>         or die_error(500, "Open git-rev-list failed");

But I was not able to fix this, at least not currently.  I wrote a failing
test case for "commit" and similar views on unborn HEAD... but they fail
_without_ error message like the one quoted.

I'd have to go slower route of examining gitweb code in how it deals with
"invalid" HEAD (i.e. HEAD not pointing to commit, being on unborn branch).

>>> For my git projects on gerrit, our main branch name is "base".
>>> We don't have any code on "master" branch.
>>> May be the $HEAD is looking for master branch(or checked out branch in git
>>> project).--> In our case, "master" is an empty branch.
>>> Also, In the git projects, the HEAD file is pointing to "ref:
>>> refs/heads/master".
>>> So, I think that's the reason for errors.
>>>
>>> How can I make $HEAD to point to a branch other than "master"?.
>>>   a) I can do this by pointing HEAD file in git projects to other branch
>> [...]
>>>   b) Is there any way, other than doing above step (a) ?.
>>>      I mean I don't want to have a code on "master" branch and also I
>>>      don't want to point HEAD file in git projects to some other branch.
>>>      Do I need to make any modifications to "gitweb.cgi" for this?
>>
>> Now that I know the source of this error, I can write test case
>> for it, and fix it.  I'll try to do it soon.
>>
>> So finally what you would need for (b) is just upgrade gitweb.

And here is the patch:
-- >8 ------------ >8 ---
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Deal with HEAD pointing to unborn branch in "heads"
 view

Gitweb has problems if HEAD points to an unborn branch, with no
commits on it yet, but there are other branches present (so it is not
newly initialized repository).

This can happen if non-bare repository (with default 'master' branch)
is updated not via committing but by other means like push to it, or
Gerrit.  It can happen also just after running "git checkout --orphan
<new branch>" but before creating any new commit on this branch.

This commit adds test and fixes the issue of being on unborn branch
(of HEAD not pointing to a commit) in "heads" view, and also in
"summary" view -- which includes "heads" excerpt as subview.

While at it rename local variable $head to $head_at, as it points to
current commit rather than current branch name (HEAD contents).

Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl                     |    4 ++--
 t/t9500-gitweb-standalone-no-errors.sh |    9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 9cf7e71..1f0ec12 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5570,7 +5570,7 @@ sub git_tags_body {
 
 sub git_heads_body {
 	# uses global variable $project
-	my ($headlist, $head, $from, $to, $extra) = @_;
+	my ($headlist, $head_at, $from, $to, $extra) = @_;
 	$from = 0 unless defined $from;
 	$to = $#{$headlist} if (!defined $to || $#{$headlist} < $to);
 
@@ -5579,7 +5579,7 @@ sub git_heads_body {
 	for (my $i = $from; $i <= $to; $i++) {
 		my $entry = $headlist->[$i];
 		my %ref = %$entry;
-		my $curr = $ref{'id'} eq $head;
+		my $curr = defined $head_at && $ref{'id'} eq $head_at;
 		if ($alternate) {
 			print "<tr class=\"dark\">\n";
 		} else {
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 0f771c6..81246a6 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -739,4 +739,13 @@ test_expect_success \
 	'echo "\$projects_list_group_categories = 1;" >>gitweb_config.perl &&
 	 gitweb_run'
 
+# ----------------------------------------------------------------------
+# unborn branches
+
+test_expect_success \
+	'unborn HEAD: "summary" page (with "heads" subview)' \
+	'git checkout orphan_branch || git checkout --orphan orphan_branch &&
+	 test_when_finished "git checkout master" &&
+	 gitweb_run "p=.git;a=summary"'
+
 test_done
-- 
1.7.9

Re: [PATCH] gitweb: Deal with HEAD pointing to unborn branch in "heads" view

[Jakub Narebski]

On Mon, 6 Feb 2012, rajesh boyapati wrote:
> 
> Thanks for your work.

I'm sorry I was able to find a fix only for the part of issue.

>>>>>>>>> [2012-01-25 18:50:35,658] ERROR
>>>>>>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Wed Jan 25
>>>>>>>>> 18:50:35 2012] gitweb.cgi: Use of uninitialized value $commit_id
>>>>>>>>> in open at /usr/lib/cgi-bin/gitweb.cgi line 2817.
>>>>>>>
>>>>>>> sub parse_commits {
>>>>>>>     my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
>>>>>>>     my @cos;
>>>>>>>
>>>>>>>     $maxcount ||= 1;
>>>>>>>     $skip ||= 0;
>>>>>>>
>>>>>>>     local $/ = "\0";
>>>>>>>
>>>>>>>     open my $fd, "-|", git_cmd(), "rev-list",
>>>>>>>         "--header",
>>>>>>>         @args,
>>>>>>>         ("--max-count=" . $maxcount),
>>>>>>>         ("--skip=" . $skip),
>>>>>>>         @extra_options,
>>>>>>>         $commit_id,
>>>>>>>         "--",
>>>>>>>         ($filename ? ($filename) : ())
>>>>>>>         or die_error(500, "Open git-rev-list failed");
>>
>> But I was not able to fix this, at least not currently.  I wrote a failing
>> test case for "commit" and similar views on unborn HEAD... but they fail
>> _without_ error message like the one quoted.
>>
>> I'd have to go slower route of examining gitweb code in how it deals with
>> "invalid" HEAD (i.e. HEAD not pointing to commit, being on unborn branch).

[...]
>> And here is the patch:
>> -->8 ------------>8 ---
>> From: Jakub Narebski <jnareb@gmail.com>
>> Subject: [PATCH] gitweb: Deal with HEAD pointing to unborn branch in
>>   "heads"  view
[...]
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 9cf7e71..1f0ec12 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -5570,7 +5570,7 @@ sub git_tags_body {
>>
>>  sub git_heads_body {
>>        # uses global variable $project
>> -       my ($headlist, $head, $from, $to, $extra) = @_;
>> +       my ($headlist, $head_at, $from, $to, $extra) = @_;
>>        $from = 0 unless defined $from;
>>        $to = $#{$headlist} if (!defined $to || $#{$headlist} < $to);
>>
> 
> I didn't see a file called "gitweb.perl" in /usr/share/gitweb/

The file "gitweb.perl", or rather "gitweb/gitweb.perl" is the name
of the script in git.git repository.  From it "make gitweb" would
generate "gitweb.cgi" file...

> I applied this patch to file "index.cgi" in /usr/share/gitweb/index.cgi at
> line 4711.
[...]
> 
> I applied this patch to file "index.cgi" in /usr/share/gitweb/index.cgi at
> line 4720.


...and I guess Gerrit build process generates "index.cgi" from that.

> Had I applied the patch to the correct file "index.cgi", which is a link to
> file "gitweb.cgi" in /usr/lib/cgi-bin/gitweb.cgi ?

Ah, right.

> Then, I restarted gerrit server to take changes.
> Now the error log of gerrit shows:

> [2012-02-06 11:21:46,726] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> 'HEAD'
> [2012-02-06 11:21:49,167] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Mon Feb  6 11:21:49
> 2012] gitweb.cgi: Use of uninitialized value $commit_id in open at
> /usr/lib/cgi-bin/gitweb.cgi line 2817.
> [2012-02-06 11:21:49,169] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision ''
[the same errors repeated few times]

> <<<<<<<<<<<<<<<<
> Previously, there is a error showing at line 4720. Now, with this patch,
> that error has gone.

As I said I was able to find a fix only for part of the issue.  
Unfortunately I was not able to reproduce this error in this form.
Note that the error location doesn't help much, because it is more
interesting for find which callers of parse_commits() pass undefined
$commit_id.

I can try to harden parse_commits() against bogus parameters; maybe
this would help.
 
> I tried to upgrade gitweb with the command "sudo apt-get install gitweb",
> but, it didn't find any upgrade.
> Am I doing in a right way?

There is no new version of gitweb yet; it haven't even been accepted
by Junio Hamano, maintainer of git of which gitweb is part, into git
repository (I might have to resend this patch for better visibility).

> Is there any place like "Github" (where we can place git projects) for
> gitweb ?

Gitweb is for quite some time developed within git repository.  From
it the 'gitweb' package is created.

Clones of canonical, official git repository can be found in a few places:

        git://git.kernel.org/pub/scm/git/git.git
        git://repo.or.cz/alt-git.git
        https://code.google.com/p/git-core/
        https://github.com/git/git

My own clone of git, with my work, can be found at:

       git://repo.or.cz/git/jnareb-git.git
       https://github.com/jnareb/git

>> diff --git a/t/t9500-gitweb-standalone-no-errors.sh
>> b/t/t9500-gitweb-standalone-no-errors.sh
>> index 0f771c6..81246a6 100755
>> --- a/t/t9500-gitweb-standalone-no-errors.sh
>> +++ b/t/t9500-gitweb-standalone-no-errors.sh
>> @@ -739,4 +739,13 @@ test_expect_success \
>>        'echo "\$projects_list_group_categories = 1;">>gitweb_config.perl
>> &&
>>         gitweb_run'
>>
>> +# ----------------------------------------------------------------------
>> +# unborn branches
>> +
>> +test_expect_success \
>> +       'unborn HEAD: "summary" page (with "heads" subview)' \
>> +       'git checkout orphan_branch || git checkout --orphan orphan_branch
>> &&
>> +        test_when_finished "git checkout master" &&
>> +        gitweb_run "p=.git;a=summary"'
>> +
>>  test_done
>>
> 
> I didn't find a file where to apply this patch.
> Is this file to test your patch for you?

Yes, this is to test that my patch fixes the issue correctly, and to
ensure that further changes don't re-break it.  It is not usually
installed with git or gitweb, so don't worry about it.

-- 
Jakub Narebski
Poland

[PATCH] gitweb: Harden parse_commit and parse_commits

[Jakub Narebski]

On Tue, 7 Feb 2012, Jakub Narebski wrote:
> On Mon, 6 Feb 2012, rajesh boyapati wrote:
[...]
> > Then, I restarted gerrit server to take changes.
> > Now the error log of gerrit shows:
> 
> > [2012-02-06 11:21:46,726] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-06 11:21:49,167] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Mon Feb  6 11:21:49
> > 2012] gitweb.cgi: Use of uninitialized value $commit_id in open at
> > /usr/lib/cgi-bin/gitweb.cgi line 2817.
> > [2012-02-06 11:21:49,169] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision ''
> [the same errors repeated few times]
> 
> > <<<<<<<<<<<<<<<<
> > Previously, there is a error showing at line 4720. Now, with this patch,
> > that error has gone.
> 
> As I said I was able to find a fix only for part of the issue.  
> Unfortunately I was not able to reproduce this error in this form.
> Note that the error location doesn't help much, because it is more
> interesting for find which callers of parse_commits() pass undefined
> $commit_id.
> 
> I can try to harden parse_commits() against bogus parameters; maybe
> this would help.

Does the following patch help, and does it fix the issue?

(Nb. you can try to simply change filename, and apply it with fuzz
against index.cgi file).
-- >8 -- ----- ----- ----- ----- ----- -- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits

Gitweb has problems and gives errors when repository it shows is on
unborn branch (HEAD doesn't point to a valid commit), but there exist
other branches.

One of errors that shows in gitweb logs is undefined $commit_id in
parse_commits() subroutine.  Therefore we harden both parse_commit()
and parse_commits() against undefined $commit_id, and against no
output from git-rev-list because HEAD doesn't point to a commit.

Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f9535eb..1181aeb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3334,6 +3334,8 @@ sub parse_commit {
 	my ($commit_id) = @_;
 	my %co;
 
+	return unless defined $commit_id;
+
 	local $/ = "\0";
 
 	open my $fd, "-|", git_cmd(), "rev-list",
@@ -3343,7 +3345,9 @@ sub parse_commit {
 		$commit_id,
 		"--",
 		or die_error(500, "Open git-rev-list failed");
-	%co = parse_commit_text(<$fd>, 1);
+	my $commit_text = <$fd>;
+	%co = parse_commit_text($commit_text, 1)
+		if defined $commit_text;
 	close $fd;
 
 	return %co;
@@ -3353,6 +3357,7 @@ sub parse_commits {
 	my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
 	my @cos;
 
+	return unless defined $commit_id;
 	$maxcount ||= 1;
 	$skip ||= 0;
 
-- 
1.7.9

Re: [PATCH] gitweb: Harden parse_commit and parse_commits

[Jakub Narebski]

Please do not remove git@vger.kernel.org (git mailing list) from Cc,
i.e. please use "Reply to all" instead of just "Reply to author".

On Wed, 8 Feb 2012, rajesh boyapati wrote:
> 2012/2/8 Jakub Narebski <jnareb@gmail.com>
 
[...]
> > Does the following patch help, and does it fix the issue?
> >
> > (Nb. you can try to simply change filename, and apply it with fuzz
> > against index.cgi file).
> > -- >8 -- ----- ----- ----- ----- ----- -- >8 --
> > From: Jakub Narebski <jnareb@gmail.com>
> > Subject: [PATCH] gitweb: Harden parse_commit and parse_commits
[...]
> When I applied the above patch and also the patch from your previous
> e-mail, I am getting this error
> >>>>>>>>>>>>>
> [2012-02-08 14:09:58,396] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> 'HEAD'
> [2012-02-08 14:10:06,732] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> 'HEAD'
> [2012-02-08 14:10:11,404] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> 'HEAD'
> [2012-02-08 14:10:15,270] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
> object name HEAD
> <<<<<<<<<<<<<<
> With these patches, the previous errors at line numbers are gone.

Thanks for information.


This final issue will be a bit harder to fix.  This error message

  fatal: bad revision 'HEAD'

comes from git (I think from "git rev-list" command), and not from gitweb.
It is printed on STDERR of git command.  What has to be done to fix it is
to capture stderr of a process, or silence it.

Unfortunately it is not that easy.  We use list form of open, which avoids
using a shell interpreter to run command, and is safer wrt. shell escaping.

The only place where gitweb cares about redirecting standard error from git
command is git_object().  It is a bit hacky, and might be not entirely safe.
To fix this issue we would have to do the same in parse_commit*() as in
git_object(), or provide some kind of wrapper like IPC::Run provides
for redirecting stderr of called command.

Note that this issue was not considered very important, because this message
doesn't goes into web server logs when running gitweb via mod_cgi with
Apache... and probably also with other web servers.  Gerrit (or rather
whatever it uses for serving CGI scripts) might be exception here.

-- 
Jakub Narebski
Poland

[PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[Jakub Narebski]

On Thu, 9 Feb 2012, Jakub Narebski wrote:
> On Wed, 8 Feb 2012, rajesh boyapati wrote:
> > 2012/2/8 Jakub Narebski <jnareb@gmail.com>
>  
> [...]
> > > Does the following patch help, and does it fix the issue?
> > >
> > > (Nb. you can try to simply change filename, and apply it with fuzz
> > > against index.cgi file).
> > > -- >8 -- ----- ----- ----- ----- ----- -- >8 --
> > > From: Jakub Narebski <jnareb@gmail.com>
> > > Subject: [PATCH] gitweb: Harden parse_commit and parse_commits
> [...]
> > When I applied the above patch and also the patch from your previous
> > e-mail, I am getting this error
> > >>>>>>>>>>>>>
> > [2012-02-08 14:09:58,396] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-08 14:10:06,732] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-08 14:10:11,404] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-08 14:10:15,270] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
> > object name HEAD
> > <<<<<<<<<<<<<<
> > With these patches, the previous errors at line numbers are gone.
> 
> Thanks for information.
> 
> 
> This final issue will be a bit harder to fix.  This error message
> 
>   fatal: bad revision 'HEAD'
> 
> comes from git (I think from "git rev-list" command), and not from gitweb.
> It is printed on STDERR of git command.  What has to be done to fix it is
> to capture stderr of a process, or silence it.
> 
> Unfortunately it is not that easy.  We use list form of open, which avoids
> using a shell interpreter to run command, and is safer wrt. shell escaping.
> 
> The only place where gitweb cares about redirecting standard error from git
> command is git_object().  It is a bit hacky, and might be not entirely safe.
> To fix this issue we would have to do the same in parse_commit*() as in
> git_object(), or provide some kind of wrapper like IPC::Run provides
> for redirecting stderr of called command.
> 
> Note that this issue was not considered very important, because this message
> doesn't goes into web server logs when running gitweb via mod_cgi with
> Apache... and probably also with other web servers.  Gerrit (or rather
> whatever it uses for serving CGI scripts) might be exception here.

Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
object name HEAD" errors for you.

I'll resend the all the patches as single patch series for inclusion in
git, but I am not sure if this latest patch will be accepted because of
drawbacks of its implementation.

-- >8 ---- ----- ----- ----- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

git-rev-list command in parse_commit() and parse_commits() can fail
because $commit_id doesn't point to a valid commit; for example if we
are on unborn branch HEAD doesn't point to a valid commit.

In this case git-rev-list prints

  fatal: bad revision 'HEAD'

on its standard error.  This commit silences this warning, at the cost
of using shell to redirect it to /dev/null, and relying on
quote_command() to protect against code injection.

Note however that such error message from git (from extrenal command)
usually but not always does not appear in web server logs.

Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1181aeb..081ac45 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3338,12 +3338,13 @@ sub parse_commit {
 
 	local $/ = "\0";
 
-	open my $fd, "-|", git_cmd(), "rev-list",
+	open my $fd, "-|", quote_command(
+		git_cmd(), "rev-list",
 		"--parents",
 		"--header",
 		"--max-count=1",
 		$commit_id,
-		"--",
+		"--") . ' 2>/dev/null',
 		or die_error(500, "Open git-rev-list failed");
 	my $commit_text = <$fd>;
 	%co = parse_commit_text($commit_text, 1)
@@ -3363,7 +3364,8 @@ sub parse_commits {
 
 	local $/ = "\0";
 
-	open my $fd, "-|", git_cmd(), "rev-list",
+	open my $fd, "-|", quote_command(
+		git_cmd(), "rev-list",
 		"--header",
 		@args,
 		("--max-count=" . $maxcount),
@@ -3371,7 +3373,7 @@ sub parse_commits {
 		@extra_options,
 		$commit_id,
 		"--",
-		($filename ? ($filename) : ())
+		($filename ? ($filename) : ())) . ' 2>/dev/null'
 		or die_error(500, "Open git-rev-list failed");
 	while (my $line = <$fd>) {
 		my %co = parse_commit_text($line);
-- 
1.7.9

Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[Jakub Narebski]

On Mon, 13 Feb 2012, rajesh boyapati wrote:
> 2012/2/11 Jakub Narebski <jnareb@gmail.com>
>> On Thu, 9 Feb 2012, Jakub Narebski wrote:
>>> On Wed, 8 Feb 2012, rajesh boyapati wrote:
>>>> 2012/2/8 Jakub Narebski <jnareb@gmail.com>

[...]
>>>> When I applied the above patch and also the patch from your previous
>>>> e-mail, I am getting this error
>>>>>>>>>>>>>>>>>
>>>> [2012-02-08 14:09:58,396] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
>>>> 'HEAD'
>>>> [2012-02-08 14:10:06,732] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
>>>> 'HEAD'
>>>> [2012-02-08 14:10:11,404] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
>>>> 'HEAD'
>>>> [2012-02-08 14:10:15,270] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
>>>> object name HEAD
>>>> <<<<<<<<<<<<<<
>>>> With these patches, the previous errors at line numbers are gone.
[...]
>>> This final issue will be a bit harder to fix.  This error message
>>>
>>>   fatal: bad revision 'HEAD'
>>>
>>> comes from git (I think from "git rev-list" command), and not from gitweb.
>>> It is printed on STDERR of git command.  What has to be done to fix it is
>>> to capture stderr of a process, or silence it.
[...]
>> Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
>> object name HEAD" errors for you.
>>
>> I'll resend the all the patches as single patch series for inclusion in
>> git, but I am not sure if this latest patch will be accepted because of
>> drawbacks of its implementation.
>>
>> -->8 ---- ----- ----- ----->8 --
>> From: Jakub Narebski <jnareb@gmail.com>
>> Subject: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
[...]
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 1181aeb..081ac45 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -3338,12 +3338,13 @@ sub parse_commit {
>>
>>        local $/ = "\0";
>>
>> -       open my $fd, "-|", git_cmd(), "rev-list",
>> +       open my $fd, "-|", quote_command(
>> +               git_cmd(), "rev-list",
>>                "--parents",
>>                "--header",
>>                "--max-count=1",
>>                $commit_id,
>> -               "--",
>> +               "--") . ' 2>/dev/null',
>>                or die_error(500, "Open git-rev-list failed");
>>        my $commit_text = <$fd>;
>>        %co = parse_commit_text($commit_text, 1)
>> @@ -3363,7 +3364,8 @@ sub parse_commits {
>>
>>        local $/ = "\0";
>>
>> -       open my $fd, "-|", git_cmd(), "rev-list",
>> +       open my $fd, "-|", quote_command(
>> +               git_cmd(), "rev-list",
>>                "--header",
>>                @args,
>>                ("--max-count=" . $maxcount),
>> @@ -3371,7 +3373,7 @@ sub parse_commits {
>>                @extra_options,
>>                $commit_id,
>>                "--",
>> -               ($filename ? ($filename) : ())
>> +               ($filename ? ($filename) : ())) . ' 2>/dev/null'
>>                or die_error(500, "Open git-rev-list failed");
>>        while (my $line = <$fd>) {
>>                my %co = parse_commit_text($line);
>>
> 
> I am getting this error with this patch
>>>>>>>>>>>>>>>>>>>
> [2012-02-13 11:20:19,268] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: usage: git rev-list
> [OPTION] <commit-id>... [ -- paths... ]
> [2012-02-13 11:20:19,268] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI:   limiting output:
> [2012-02-13 11:20:19,268] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI:     --max-count=nr
[...]
> [2012-02-13 11:20:27,913] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
> used after filename
> [2012-02-13 11:20:32,579] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
> used after filename
> <<<<<<<<<<<<<<<<<<<

Strange, I cannot reproduce this with non-Gerrit gitweb.  It looks
like it somehow lost in between "... -- 2>/dev/null" at the end of
git-rev-list command, and fails at "--2" which is bad flag.

Are you sure you applied the patch correctly?  Does 'object' view
(take any 'commit' or 'blob' or 'tree' view, and replace action part
by 'object') works correctly in Gerrit's gitweb?

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[Jakub Narebski]

On Mon, 13 Feb 2012, rajesh boyapati wrote:
> 2012/2/13 Jakub Narebski <jnareb@gmail.com>
>> On Mon, 13 Feb 2012, rajesh boyapati wrote:

>>> I am getting this error with this patch
>>>>>>>>>>>>>>>>>>>>>
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: usage: git rev-list
>>> [OPTION] <commit-id>... [ -- paths... ]
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI:   limiting output:
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI:     --max-count=nr
>> [...]
>>> [2012-02-13 11:20:27,913] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
>>> used after filename
>>> [2012-02-13 11:20:32,579] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
>>> used after filename
>>> <<<<<<<<<<<<<<<<<<<
>>
>> Strange, I cannot reproduce this with non-Gerrit gitweb.  It looks
>> like it somehow lost in between "... -- 2>/dev/null" at the end of
>> git-rev-list command, and fails at "--2" which is bad flag.
>>
> This is the patch I applied
>>>>>>>>>>>>>
> sub parse_commit {
>     my ($commit_id) = @_;
>     my %co;
> 
>         return unless defined $commit_id;
> 
>     local $/ = "\0";
> 
>     *open my $fd, "-|", quote_command(
>              git_cmd(), "rev-list",*
>         "--parents",
>         "--header",
>         "--max-count=1",
>         $commit_id,
>        * "--") . '2>/dev/null',*
                   ^^^^^^^^^^^^^

It should be ' 2>/dev/null', with space before redirection, and not
'2>/dev/null'.  This space is here necessary.

> With this Patch, Gerrit's gitweb is not showing anything.
> I mean, I can access gitweb from gerrit, but if I click on Tabs(like log,
> commit, etc...which worked with previous patches), I cannot see any thing.
> 
> Even with previous patches also there is no improvement in Gerrit's gitweb,
> only some of the errors are gone in error_log.
> The improvement I am talking about is "If I click other tabs(log, shortlog,
> commit, tree,etc) after clicking "summary", Gerrit's gitweb is not showing
> anything".

Many views in gitweb do default to HEAD.  If HEAD does not point to a valid
commit, they would fail, in better or worse way.

Except for the first one those patches are more of band-aid and workaround
than fixing underlying issue that gitweb assumes that HEAD is valid in 
non-empty repository.  But fixing this will require more work.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[Jakub Narebski]

On Mon, 13 Feb 2012, rajesh boyapati wrote:
> 2012/2/13 Jakub Narebski <jnareb@gmail.com>
>> On Mon, 13 Feb 2012, rajesh boyapati wrote:


>>> This is the patch I applied
>>>>>>>>>>>>>>>
>>> sub parse_commit {
>>>     my ($commit_id) = @_;
>>>     my %co;
>>>
>>>         return unless defined $commit_id;
>>>
>>>     local $/ = "\0";
>>>
>>>     *open my $fd, "-|", quote_command(
>>>              git_cmd(), "rev-list",*
>>>         "--parents",
>>>         "--header",
>>>         "--max-count=1",
>>>         $commit_id,
>>>        * "--") . '2>/dev/null',*
>>                   ^^^^^^^^^^^^^
>>
>> It should be ' 2>/dev/null', with space before redirection, and not
>> '2>/dev/null'.  This space is here necessary.
> 
> 
> Sorry for the typo error. I changed that and I am now seeing this error in
> Gerrit's error_log
> <<<<<<<<<<<<<
> [2012-02-13 13:45:35,201] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
> object name HEAD
> >>>>>>>>>>>>>

WTF, redirecting stderr (file descriptor number 2) to /dev/null should have
silenced git-rev-list in case of errors.

Compare

  $ git rev-list foo --
  fatal: bad revision 'foo'

with

  $ git rev-list foo -- 2>/dev/null

which gives no output.

I wonder what shell does gitweb use if run from com.google.gerrit.httpd
as servlet...

[...]
>> Many views in gitweb do default to HEAD.  If HEAD does not point to a valid
>> commit, they would fail, in better or worse way.
>>
>> Except for the first one those patches are more of band-aid and workaround
>> than fixing underlying issue that gitweb assumes that HEAD is valid in
>> non-empty repository.  But fixing this will require more work.
>>
> Yes, I agree with you.
>
> If the HEAD's in a git project are pointed to master branch, those are fine
> with Gerrits gitweb. Everything is working fine. (See the images in attached
> file "Master.zip")
> 
> For the HEAD's in the git projects that are pointed to a branch other than
> master (I mean if master branch is empty), I have the problem when I click
> on tabs (log, shortlog, commit, commitdiff, tree) after clicking "summary"
> tab.  (See the images in attched file "Unborn-branch.zip").

That is because those views defaults to HEAD, which doesn't point to
a valid commit because 'master' branch it does point to doesn't have any
commits on it.  They should probably be disabled (grayed-out, and made
ordinary text and not hyperlink) if HEAD is invalid.

But this would require more work than those patches.
>
> If I click other tabs before clicking "summary" tab, they are working fine.

That is because if you select some branch, then all those views use
currently selected branch (passed via URL, e.g. 'h' or 'hb' parameter
in case of query-params URL).

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
> object name HEAD" errors for you.

I have to wonder if it is simpler and less error prone to check HEAD
before doing anything else immediately after which repository is being
consulted, and give the same "no history at all yet in this project" page
for most if not all operations, instead of patching things up at this deep
in the callchain.

Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
> > object name HEAD" errors for you.
> 
> I have to wonder if it is simpler and less error prone to check HEAD
> before doing anything else immediately after which repository is being
> consulted, and give the same "no history at all yet in this project" page
> for most if not all operations, instead of patching things up at this deep
> in the callchain.

Actually the problem is that there is history (there are other branches),
but HEAD points to unborn (unused) 'master' branch.

But you are right that we should fix underlying issue with invalid
assumption which gitweb uses, that HEAD points to a valid commit if
repository is non-empty.

Though I think leaving safety of 2nd patch could be a good idea anyway.
And 1st patch fixes a real issue, and does not only provide band-aid.


P.S. I will resend those three patches as patch series for easier review,
so they are not tangled in this deep thread.  They should be in 'gitweb/web'
branch in my public forks of git repository on repo.or.cz and github.

-- 
Jakub Narebski
Poland