Re: [PATCH 2/3] make update-server-info more robust

["René Scharfe" <l.s.r@web.de>]

Am 13.09.2014 um 22:19 schrieb Jeff King:
> Since "git update-server-info" may be called automatically
> as part of a push or a "gc --auto", we should be robust
> against two processes trying to update it simultaneously.
> However, we currently use a fixed tempfile, which means that
> two simultaneous writers may step on each other's toes and
> end up renaming junk into place.
> 
> Let's instead switch to using a unique tempfile via mkstemp.
> We do not want to use a lockfile here, because it's OK for
> two writers to simultaneously update (one will "win" the
> rename race, but that's OK; they should be writing the same
> information).
> 
> While we're there, let's clean up a few other things:
> 
>    1. Detect write errors. Report them and abort the update
>       if any are found.
> 
>    2. Free path memory rather than leaking it (and clean up
>       the tempfile when necessary).
> 
>    3. Use the pathdup functions consistently rather than
>       static buffers or manually calculated lengths.
> 
> This last one fixes a potential overflow of "infofile" in
> update_info_packs (e.g., by putting large junk into
> $GIT_OBJECT_DIRECTORY). However, this overflow was probably
> not an interesting attack vector for two reasons:
> 
>    a. The attacker would need to control the environment to
>       do this, in which case it was already game-over.
> 
>    b. During its setup phase, git checks that the directory
>       actually exists, which means it is probably shorter
>       than PATH_MAX anyway.
> 
> Because both update_info_refs and update_info_packs share
> these same failings (and largely duplicate each other), this
> patch factors out the improved error-checking version into a
> helper function.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess point (b) may not apply on systems that have a really small
> PATH_MAX that does not reflect what you can actually create in the
> filesystem (Windows?).

It's the other way around: PATH_MAX is an actual limit basically only
on Windows [1] unless you avoid using the Windows API [2].

Regardless of the security implications, getting rid of more PATH_MAX
buffers is a good move.

And I looked only briefly at your patch, but I like the three bullet
points above. :)

René


[1] http://insanecoding.blogspot.de/2007/11/pathmax-simply-isnt.html
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx