Re: [PATCH] Fix buffer overflow in git-grep

[Dmitry Potapov <dpotapov@gmail.com>]

On Wed, Jul 16, 2008 at 12:35:06PM +0200, Johannes Schindelin wrote:
> >  
> >  	while (tree_entry(tree, &entry)) {
> > -		strcpy(path_buf + len, entry.path);
> > +		int te_len = tree_entry_len(entry.path, entry.sha1);
> > +		if (len + te_len >= PATH_MAX + tn_len)
> > +			die ("path too long: %s", path_buf+tn_len);
> > +		memcpy(path_buf + len, entry.path, te_len);
> 
> That is brutal.  Does grep_tree() not work on tree objects in memory?  In 
> that case, you prevent the user from grepping, only because she is on a 
> suboptimal platform, _even if_ even that platform could cope with it.

Sure, but other git commands do not work much better in this case.
In fact, what you called as "brutal" may be considered as very
polite comparing to what other git commands did.

For instance, git show will show you nothing at all and exit with 0.
The same problem with git whatchanged. The whole history mysteriously
disappeared at that commit, and git whatchanged exited with 0 without
any error or warning... Though git log will show you all history, but
if you run it with -p then it will also exit with zero at this commit
silently like previously history do not exist at all. So, I didn't see
any reason to make git grep to work in the situation where practically
any other git command does not. I guess, they should be corrected too,
but I did not have time to look at them yet.

> 
> It's not like the path is ever used to access a file, right?
> 
> Maybe you should convert the path_buf to a strbuf instead.

It is probably a good suggestion, but I just wanted to provided a quick
fix to what may be considered as security issue. Of course, you usually
do not grep on untrusted repos, but if you did and something nasty
happened to you. I don't think it will help Git's reputation as being
secure and reliable...

Now the question is whether we really want to fix all Git commands that
do not touch the work tree to work with filenames longer than PATH_MAX?

Dmitry