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

On Thu, 9 Feb 2012, Jakub Narebski wrote:

On Wed, 8 Feb 2012, rajesh boyapati wrote:

quoted

2012/2/8 Jakub Narebski [off-list ref]

 
[...]

quoted

quoted

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 <redacted>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits

[...]

quoted

When I applied the above patch and also the patch from your previous
e-mail, I am getting this error

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

[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 <redacted>
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 <redacted>
Signed-off-by: Jakub Narebski <redacted>
---
 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