Re: [PATCH] Prevent buffer overflows when path is too big

(may be s/path is too big/path is too long/ ?)

On 19.10.13 12:52, Antoine Pelisse wrote:

I think this should be if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'

I'm not sure how to handle overlong path in general, there are several ways: a) Silently overwrite memory (with help of memcpy() and/or strcpy() b) Silently shorten the path using strlcpy() instead of strcpy() c) Avoid the overwriting and call die(). d) Prepare a longer buffer using xmalloc()

Today we do a), this is not a good thing and the worst choice.

A little side note:
  It would be good to have test cases for either b), c) or d).

  As PATH_MAX is OS dependend, we need both a main program written in c
  and a test case written in t/txxxx.sh.
  Some existing code can be used for inspiration, e.g. 
  test-wildmatch.c in combination with t/t3070-wildmatch.sh
  This willl allow us to reproduce the error, and define how git should behave.

End of the side note, let's look closer at the suggested patch, implementing b)

Silently shortening an overlong path like "/foo/bar/baz" could result something like

"/foo/bar/ba" /* That filename may be part of the repo too */ or "/foo/bar/" /* This is a directory, not a file name */

In either case the end user has no idea why git choose another file name. And this could be hard to debug. After a couple of hours she/he may send a message asking for help to the mailing list, and we end up in more people doing debugging.

c) Is much easier to debug:
  Git can not handle this situation, and we print out the parameters in die()

I would prefer c) over b), make clear that git can't handle that situation.

d) Would mean some more re-factoring: Check all callers to prefix_filename(). Some of them call xstrdup() after prefix_filename(), which mean that we could change prefix_filename() to always return new string which is long enough via xmalloc(), and not a static buffer.

So we come to the next point (and this is my personal experience, so please don't get me wrong): how much time can you spend on this?

If the answer is kind of "very little", I would go for c)
  Avoid the silent memory corruption, and say to the user "we can not handle this"

If the answer is kind of "little", I would go for c) and a test program,
  covering all the different code path in abspath()
  (WHich may deserve a refactoring as well, since the code for GIT_WINDOWS_NATIVE
  is very similar to the non-GIT_WINDOWS_NATIVE)

If the answer is kind of "more than little", a different strategie may be better:
  Start sending a patch for c)
  I think we have enough volunteers here for a review, so we can life without the test code.
  On top of that, some volunteer can develop d).
  
So far I have only looked at abspath(), and your patch touches more places.
I think more and more that calling die()
with all information included why we call die() is a good starting point.

It will allow the users to see what is going on.
May be the repo can be re-arranged to use shorter path names than what we can handle.
[snip]
 
/Torsten