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

[PATCH 0/4] fix packed-refs races

From
Jeff King <peff@peff.net>
Date
May 7, 2013, 02:36 UTC
Message-ID
<20130507023610.GA22053@sigill.intra.peff.net>
In-Reply-To
<20130503083847.GA16542@sigill.intra.peff.net>
This fixes the races I brought up in the surrounding thread:
  http://thread.gmane.org/gmane.comp.version-control.git/223299

The individual races are describe in more detail in each commit, but for reference, here is the complete reproduction recipe (which I posted already in several parts throughout the thread, but is collected here):

  # base.sh
  # run this script forever in one terminal
  git init -q repo &&
  cd repo &&
  git commit -q --allow-empty -m one &&
  one=`git rev-parse HEAD` &&
  git commit -q --allow-empty -m two &&
  two=`git rev-parse HEAD` &&
  sha1=$one &&
  while true; do
    # this re-creates the loose ref in .git/refs/heads/master
    if test "$sha1" = "$one"; then
      sha1=$two
    else
      sha1=$one
    fi &&
    git update-ref refs/heads/master $sha1 &&
    # we can remove packed-refs safely, as we know that
    # its only value is now stale. Real git would not do
    # this, but we are simulating the case that "master"
    # simply wasn't included in the last packed-refs file.
    rm -f .git/packed-refs &&
    # and now we repack, which will create an up-to-date
    # packed-refs file, and then delete the loose ref
    git pack-refs --all --prune
  done
  # lookup.sh
  # run this script simultaneously in another terminal; when it exits,
  # git is broken (it erroneously told us that master did not exist).
  # It is fixed by applying up to patch 3/4 below.
  cd repo &&
  while true; do
    ref=`git rev-parse --verify master`
    echo "==> $ref"
    test -z "$ref" && break
  done
  # enumerate.sh
  # run this script simultaneously in another terminal (either with
  # lookup.sh running, too, or just base.sh); when it exits, we
  # for-each-ref has erroneously missed master. It is fixed by applying
  # up to patch 4/4 below.
  cd repo &&
  while true; do
    refs=`git.compile for-each-ref --format='%(refname)'`
    echo "==> $refs"
    test -z "$refs" && break
  done

I don't think is a purely hypothetical race. About once a month for the past six months or so, I'll run across a corrupted repository on github.com that is inexplicably missing a few objects near the tip of a ref. This last time, I happened to catch something very odd in our ref audit-log. The log for the repo that became corrupted showed a small push to the tip of "master", but the objects for that push were missing (the previous ref tip was fine). At almost the exact same time, our alternates repo was running a "fetch --prune" from the corrupted repo. But it didn't see the corrupted master ref; it saw that refs/heads/master didn't exist at all, and it deleted it.

I believe that git-upload-pack hit the enumeration race above, and showed a ref advertisement in which refs/heads/master was missing[1], leading fetch to delete the ref. At the same time, a gc was running in the corrupted repo which hit the same issue, and ended up pruning[2] the newly pushed objects, which were not referenced from anywhere else.

It may seem unlikely for two programs to hit the race at the same time (especially given the number of iterations you need to trigger the race with the scripts above), but I think it's exacerbated when you have a very large number of refs (because the loose enumeration takes much longer). So I think there's a good chance this was my problem. And if not, it is good to fix anyway. :)

  [1/4]: resolve_ref: close race condition for packed refs
  [2/4]: add a stat_validity struct
  [3/4]: get_packed_refs: reload packed-refs file when it changes
  [4/4]: for_each_ref: load all loose refs before packed refs
-Peff
[1] Usually the enumeration race would cause you to see the old value
    from the packed-refs file, not a completely missing ref (unless the
    ref was not packed at all previously, but that is almost certainly
    not the case here). But it does call into resolve_ref to actually
    read the ref, which has its own race that can cause refs to
    "disappear" (and is fixed by patch 1).
    As an aside, I think that calling resolve_ref is probably wrong. We
    know we have a fully qualified refname, because we keep track of
    which directory we are calling readdir() on. But if for whatever
    reason somebody deletes it at the exact moment between when we see
    it in readdir() and when we try to open it (not packs it, but
    actually deletes it), then we'll see it is missing and fall back to
    trying other ref-lookups. So in theory a race with a delete of
    refs/heads/foo could accidentally retrieve the value for
    refs/heads/refs/heads/foo.
    It's quite unlikely, though, and I'm not too concerned about it.
[2] Recently pushed objects should still be safe, even if they are
    unreachable. However, in our case the pruning was exacerbated by a
    gc codepath that could call "git repack -ad" (not in upstream git,
    but we do our own custom gc due to the alternates structure of our
    forks). I've also fixed that, as we should always be using "-A" to
    keep recent unreachable objects.
Previous: Michael HaggertyNext: Jeff King
Message 15 of 45 in “another packed-refs race”
  1. Jeff KingMay 3, 2013
  2. Johan HerlandMay 3, 2013
  3. Jeff KingMay 3, 2013
  4. Jeff KingMay 3, 2013
  5. Johan HerlandMay 3, 2013
  6. Michael HaggertyMay 6, 2013
  7. Jeff KingMay 6, 2013
  8. Jeff KingMay 3, 2013
  9. Michael HaggertyMay 6, 2013
  10. Jeff KingMay 6, 2013
  11. Jeff KingMay 6, 2013
  12. Michael HaggertyMay 7, 2013
  13. Jeff KingMay 7, 2013
  14. Michael HaggertyMay 7, 2013
  15. 0/4 fix packed-refs racesJeff King, May 7, 2013
  16. 1/4 resolve_ref: close race condition for packed refsJeff King, May 7, 2013
  17. Michael HaggertyMay 12, 2013
  18. Jeff KingMay 16, 2013
  19. Michael HaggertyMay 16, 2013
  20. Michael HaggertyMay 12, 2013
  21. 0/4 Fix a race condition when reading loose refsMichael Haggerty, Jun 11, 2013
  22. 1/4 resolve_ref_unsafe(): extract function handle_missing_loose_ref()Michael Haggerty, Jun 11, 2013
  23. 2/4 resolve_ref_unsafe(): handle the case of an SHA-1 within loopMichael Haggerty, Jun 11, 2013
  24. 3/4 resolve_ref_unsafe(): nest reference-reading code in an infinite loopMichael Haggerty, Jun 11, 2013
  25. 4/4 resolve_ref_unsafe(): close race condition reading loose refsMichael Haggerty, Jun 11, 2013
  26. Jeff KingJun 12, 2013
  27. Thomas RastJun 13, 2013
  28. Michael HaggertyJun 14, 2013
  29. Junio C HamanoJun 11, 2013
  30. 2/4 add a stat_validity structJeff King, May 7, 2013
  31. Michael HaggertyMay 13, 2013
  32. 0/2 Separate stat_data from cache_entryMichael Haggerty, May 13, 2013
  33. 1/2 Extract a struct stat_data from cache_entryMichael Haggerty, May 13, 2013
  34. 2/2 add a stat_validity structMichael Haggerty, May 13, 2013
  35. Junio C HamanoMay 13, 2013
  36. Jeff KingMay 16, 2013
  37. 3/4 get_packed_refs: reload packed-refs file when it changesJeff King, May 7, 2013
  38. 0/2 peel_ref cleanups changesJeff King, May 7, 2013
  39. 1/2 peel_ref: rename "sha1" argument to "peeled"Jeff King, May 7, 2013
  40. 2/2 peel_ref: refactor for safety with simultaneous updateJeff King, May 7, 2013
  41. Eric SunshineMay 9, 2013
  42. Michael HaggertyMay 13, 2013
  43. 4/4 for_each_ref: load all loose refs before packed refsJeff King, May 7, 2013
  44. Junio C HamanoMay 7, 2013
  45. Jeff KingMay 7, 2013

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.