git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
René Scharfe <l.s.r@web.de>
Date
Sep 14, 2014, 17:38 UTC
Message-ID
<5415D289.9080507@web.de>
In-Reply-To
<20140913201920.GB27082@peff.net>
Am 13.09.2014 um 22:19 schrieb Jeff King:
Show 46 quoted lines
> 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

Previous: Jeff KingNext: Junio C Hamano
Message 6 of 9 in “repack: call prune_packed_objects() and update_server_info() directly”
  1. repack: call prune_packed_objects() and update_server_info() directlyRené Scharfe, Sep 13, 2014
  2. Stefan BellerSep 13, 2014
  3. Jeff KingSep 13, 2014
  4. 1/3 prune-packed: fix minor memory leakJeff King, Sep 13, 2014
  5. 2/3 make update-server-info more robustJeff King, Sep 13, 2014
  6. René ScharfeSep 14, 2014
  7. Junio C HamanoSep 15, 2014
  8. Jeff KingSep 15, 2014
  9. 3/3 server-info: clean up after writing info/packsJeff King, Sep 13, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.