[PATCH] gitweb: Silence stderr in parse_commit*() subroutines
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Feb 11, 2012, 13:02 UTC
- Message-ID
- <201202111402.31684.jnareb@gmail.com>
- In-Reply-To
- <201202092114.40832.jnareb@gmail.com>
On Thu, 9 Feb 2012, Jakub Narebski wrote:
Show 54 quoted lines
> 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