On Thu, 9 Feb 2012, Jakub Narebski wrote:
> On Wed, 8 Feb 2012, rajesh boyapati wrote:
> > 2012/2/8 Jakub Narebski <jnareb@xxxxxxxxx>
>
> [...]
> > > 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@xxxxxxxxx>
> > > 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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Jakub Narebski <jnareb@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html