Gitweb error

PATCH Superseded Jakub Narebski 3 participants 14 messages Jun 15, 2016 9 messages not in the archive

Share & tools

A message that is not in the archive (sent off-list or not yet received): 5fa08a8b-f0a2-4796-bf0d-06a8f13bf703@b23g2000yqn.googlegroups.com

rajesh boyapati

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

Fwd: Git-web error

---------- Forwarded message ----------
From: rajesh boyapati <[email protected]>
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.

Jakub Narebski

author

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

Quoted 11 lines · rajesh boyapati

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.

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 4 lines

[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.

Quoted 3 lines

[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.

Signature · 1 line

Jakub Narebski

A message that is not in the archive (sent off-list or not yet received): CA+EqV8w5qz+iwg_PPB4M5Q-LS48B=yncR9UdR-r58BLtAEPPrA@mail.gmail.com

Jakub Narebski

author

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

Quoted 9 lines · rajesh boyapati

Quoted 8 lines · Jakub Narebski
Quoted 6 lines · rajesh boyapati
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.

Quoted 11 lines

Quoted 10 lines
Quoted 6 lines
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?

Quoted 11 lines

    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?

[...]

Signature · 2 lines

Jakub Narebski
Poland

A message that is not in the archive (sent off-list or not yet received): CA+EqV8xB6vcDrqM3EY7uRfu0c7sOj6FbMXci+5w2qgi5RSWrbw@mail.gmail.com

Jakub Narebski

author

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

Quoted 21 lines · rajesh boyapati

2012/1/28 Jakub Narebski <[email protected]>
Quoted 20 lines · rajesh boyapati
Quoted 12 lines
    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.

Quoted 10 lines

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

[...]

Quoted 4 lines

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.

Signature · 2 lines

Jakub Narebski
Poland

A message that is not in the archive (sent off-list or not yet received): CA+EqV8y3dhR8+PJbMxMNEsGjDOx6dxtPYjn8kDvAZxCAO7iS5w@mail.gmail.com

Jakub Narebski

author

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

PATCH 1/1 gitweb: Deal with HEAD pointing to unborn branch in "heads" view

Quoted 3 lines · rajesh boyapati

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.

Quoted 21 lines

2012/1/30 Jakub Narebski <[email protected]>
Quoted 20 lines · rajesh boyapati
Quoted 19 lines
2012/1/28 Jakub Narebski <[email protected]>
Quoted 18 lines · rajesh boyapati
Quoted 12 lines
    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.

Quoted 14 lines

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
[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";

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
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).

Quoted 16 lines

Quoted 16 lines
Quoted 10 lines
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
[...]
Quoted 4 lines
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?

Quoted 4 lines

Quoted 4 lines
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 <[email protected]>
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 <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

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

gitweb/gitweb.perl +2 -2

index 9cf7e71..1f0ec12 100755
@@ -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 {

t/t9500-gitweb-standalone-no-errors.sh +9 -0

index 0f771c6..81246a6 100755
@@ -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

Signature · 1 line

1.7.9

A message that is not in the archive (sent off-list or not yet received): CA+EqV8w6k2VrEtMydhGKZHbQdXHxCE3WA_0rtS-AY4cmQvii=A@mail.gmail.com

Jakub Narebski

author

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

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

Quoted 2 lines · rajesh boyapati

Thanks for your work.

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

Quoted 31 lines

Quoted 31 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
[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";
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
    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).

[...]

Quoted 5 lines

Quoted 5 lines
And here is the patch:
-->8 ------------>8 ---
From: Jakub Narebski <[email protected]>
Subject: [PATCH] gitweb: Deal with HEAD pointing to unborn branch in
"heads" view

[...]

Quoted 6 lines

Quoted 6 lines
gitweb/gitweb.perl +0 -0
index 9cf7e71..1f0ec12 100755
@@ -5570,7 +5570,7 @@ sub git_tags_body {
 

Quoted 9 lines

gitweb/gitweb.perl sub git_tags_body { · line 5576

Quoted 7 lines

gitweb/gitweb.perl +1 -1

 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...

Quoted 2 lines

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

[...]

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 12 lines

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]

Quoted 3 lines

<<<<<<<<<<<<<<<<
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.

Quoted 3 lines

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).

Quoted 2 lines

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

Quoted 10 lines

t/t9500-gitweb-standalone-no-errors.sh +0 -0

index 0f771c6..81246a6 100755
@@ -739,4 +739,13 @@ test_expect_success \
       'echo "\$projects_list_group_categories = 1;">>gitweb_config.perl

&&
gitweb_run'

Quoted 15 lines

t/t9500-gitweb-standalone-no-errors.sh test_expect_success \ · line 751
Quoted 12 lines
t/t9500-gitweb-standalone-no-errors.sh +6 -0
+# ----------------------------------------------------------------------
+# unborn branches
+
+test_expect_success \
+       'unborn HEAD: "summary" page (with "heads" subview)' \
+       'git checkout orphan_branch || git checkout --orphan orphan_branch
&&
t/t9500-gitweb-standalone-no-errors.sh +3 -0
+        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.

Signature · 2 lines

Jakub Narebski
Poland

Jakub Narebski

author

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

PATCH 1/1 gitweb: Harden parse_commit and parse_commits

Quoted 1 line · Jakub Narebski

On Mon, 6 Feb 2012, rajesh boyapati wrote:

[...]

Quoted 18 lines

Quoted 12 lines
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]
Quoted 3 lines
<<<<<<<<<<<<<<<<
Previously, there is a error showing at line 4720. Now, with this patch, that error has gone.

Quoted 8 lines

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 <[email protected]>
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 <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
 gitweb/gitweb.perl |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

gitweb/gitweb.perl +6 -1

index f9535eb..1181aeb 100755
@@ -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;

Signature · 1 line

1.7.9

A message that is not in the archive (sent off-list or not yet received): CA+EqV8xiLYo8XE--c1QfuXdhentUFpHqfPYXHt72eCpEA_hCNQ@mail.gmail.com

Jakub Narebski

author

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

Re: [PATCH] gitweb: Harden parse_commit and parse_commits

Please do not remove [email protected] (git mailing list) from Cc, i.e. please use "Reply to all" instead of just "Reply to author".

Quoted 1 line · rajesh boyapati

2012/2/8 Jakub Narebski <[email protected]>

[...]

Quoted 7 lines

Quoted 7 lines
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 <[email protected]>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits

[...]

Quoted 17 lines

When I applied the above patch and also the patch from your previous e-mail, I am getting this error
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
[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.

Signature · 2 lines

Jakub Narebski
Poland

Jakub Narebski

author

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

PATCH 1/1 gitweb: Silence stderr in parse_commit*() subroutines

Quoted 50 lines · Jakub Narebski

Quoted 1 line · rajesh boyapati
2012/2/8 Jakub Narebski <[email protected]>
[...]
Quoted 7 lines
Quoted 7 lines
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 <[email protected]>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits
[...]
Quoted 17 lines
When I applied the above patch and also the patch from your previous e-mail, I am getting this error
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
[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.

Quoted 4 lines

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 <[email protected]>
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 <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

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

gitweb/gitweb.perl +6 -4

index 1181aeb..081ac45 100755
@@ -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);

Signature · 1 line

1.7.9

Junio C Hamano

2016-06-15 22:53 UTC #15 lore raw

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

Quoted 2 lines · Jakub Narebski

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.

Jakub Narebski

author

2016-06-15 22:53 UTC #16 lore raw

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

Quoted 10 lines · Junio C Hamano

Quoted 2 lines · Jakub Narebski
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.

Signature · 2 lines

Jakub Narebski
Poland

A message that is not in the archive (sent off-list or not yet received): CA+EqV8xTsavQFWsoijrt+0UcfxSZO2voL=CawrRPvDeB=qHQfg@mail.gmail.com

Jakub Narebski

author

2016-06-15 22:53 UTC #18 lore raw

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

Quoted 4 lines · rajesh boyapati

2012/2/11 Jakub Narebski <[email protected]>
Quoted 3 lines · Jakub Narebski
Quoted 2 lines · rajesh boyapati
Quoted 1 line
2012/2/8 Jakub Narebski <[email protected]>

[...]

Quoted 17 lines

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
When I applied the above patch and also the patch from your previous e-mail, I am getting this error
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
[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.

[...]

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
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.

[...]

Quoted 10 lines

Quoted 10 lines
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 <[email protected]>
Subject: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines

[...]

Quoted 50 lines

Quoted 32 lines

gitweb/gitweb.perl +5 -3

index 1181aeb..081ac45 100755
@@ -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),

gitweb/gitweb.perl sub parse_commits { · line 3379

Quoted 8 lines

gitweb/gitweb.perl +1 -1

@@ -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

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

[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

[...]

Quoted 7 lines

[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?

Signature · 2 lines

Jakub Narebski
Poland

A message that is not in the archive (sent off-list or not yet received): CA+EqV8xin_ubOoGouhHz2qnzoHrpMMQsjUTXnrtmsxRTLPZtZQ@mail.gmail.com

Jakub Narebski

author

2016-06-15 22:53 UTC #20 lore raw

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

Quoted 27 lines · rajesh boyapati

2012/2/13 Jakub Narebski <[email protected]>
Quoted 20 lines · rajesh boyapati
Quoted 10 lines
I am getting this error with this patch
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
[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
[...]
Quoted 7 lines
[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
<<<<<<<<<<<<<<<<<<<
Quoted 4 lines
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Quoted 15 lines

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.

Quoted 9 lines

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.

Signature · 2 lines

Jakub Narebski
Poland

A message that is not in the archive (sent off-list or not yet received): CA+EqV8w5jCHa2NY+NLaht901Qk=kQvALG3EA6BkePiGow3YFeQ@mail.gmail.com

Jakub Narebski

author

2016-06-15 22:53 UTC #22 lore raw

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

Quoted 27 lines · rajesh boyapati

2012/2/13 Jakub Narebski <[email protected]>
Quoted 20 lines · rajesh boyapati
Quoted 4 lines
This is the patch I applied
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 15 lines
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',*
Quoted 4 lines
                  ^^^^^^^^^^^^^
It should be ' 2>/dev/null', with space before redirection, and not '2>/dev/null'. This space is here necessary.

Quoted 7 lines

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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

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...

[...]

Quoted 9 lines

Quoted 7 lines
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.

Quoted 8 lines

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.

Quoted 2 lines

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).

Signature · 2 lines

Jakub Narebski
Poland