Re: [PATCH] gitweb: Harden parse_commit and parse_commits

From: Jakub Narebski <jnareb@gmail.com>
Date: Feb 9, 2012, 20:14 UTC
Message-ID: <201202092114.40832.jnareb@gmail.com>
In-Reply-To: <CA+EqV8xiLYo8XE--c1QfuXdhentUFpHqfPYXHt72eCpEA_hCNQ@mail.gmail.com>

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>

 
[...]

Show 7 quoted 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 <jnareb@gmail.com>
> > Subject: [PATCH] gitweb: Harden parse_commit and parse_commits

[...]

Show 17 quoted lines

> 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