From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
Johan Herland <johan@herland.net>,
Junio C Hamano <gitster@pobox.com>
Subject: [PATCH 0/4] fix packed-refs races
Date: Mon, 6 May 2013 22:36:11 -0400 [thread overview]
Message-ID: <20130507023610.GA22053@sigill.intra.peff.net> (raw)
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.
next prev parent reply other threads:[~2013-05-07 2:36 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-03 8:38 another packed-refs race Jeff King
2013-05-03 9:26 ` Johan Herland
2013-05-03 17:28 ` Jeff King
2013-05-03 18:26 ` Jeff King
2013-05-03 21:02 ` Johan Herland
2013-05-06 12:12 ` Michael Haggerty
2013-05-06 18:44 ` Jeff King
2013-05-03 21:21 ` Jeff King
2013-05-06 12:03 ` Michael Haggerty
2013-05-06 18:41 ` Jeff King
2013-05-06 22:18 ` Jeff King
2013-05-07 4:32 ` Michael Haggerty
2013-05-07 4:44 ` Jeff King
2013-05-07 8:03 ` Michael Haggerty
2013-05-07 2:36 ` Jeff King [this message]
2013-05-07 2:38 ` [PATCH 1/4] resolve_ref: close race condition for packed refs Jeff King
2013-05-12 22:56 ` Michael Haggerty
2013-05-16 3:47 ` Jeff King
2013-05-16 5:50 ` Michael Haggerty
2013-05-12 23:26 ` Michael Haggerty
2013-06-11 14:26 ` [PATCH 0/4] Fix a race condition when reading loose refs Michael Haggerty
2013-06-11 14:26 ` [PATCH 1/4] resolve_ref_unsafe(): extract function handle_missing_loose_ref() Michael Haggerty
2013-06-11 14:26 ` [PATCH 2/4] resolve_ref_unsafe(): handle the case of an SHA-1 within loop Michael Haggerty
2013-06-11 14:26 ` [PATCH 3/4] resolve_ref_unsafe(): nest reference-reading code in an infinite loop Michael Haggerty
2013-06-11 14:26 ` [PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs Michael Haggerty
2013-06-12 8:04 ` Jeff King
2013-06-13 8:22 ` Thomas Rast
2013-06-14 7:17 ` Michael Haggerty
2013-06-11 20:57 ` [PATCH 0/4] Fix a race condition when " Junio C Hamano
2013-05-07 2:39 ` [PATCH 2/4] add a stat_validity struct Jeff King
2013-05-13 2:29 ` Michael Haggerty
2013-05-13 3:00 ` [RFC 0/2] Separate stat_data from cache_entry Michael Haggerty
2013-05-13 3:00 ` [RFC 1/2] Extract a struct " Michael Haggerty
2013-05-13 3:00 ` [RFC 2/2] add a stat_validity struct Michael Haggerty
2013-05-13 5:10 ` [RFC 0/2] Separate stat_data from cache_entry Junio C Hamano
2013-05-16 3:51 ` [PATCH 2/4] add a stat_validity struct Jeff King
2013-05-07 2:43 ` [PATCH 3/4] get_packed_refs: reload packed-refs file when it changes Jeff King
2013-05-07 2:54 ` [PATCH 0/2] peel_ref cleanups changes Jeff King
2013-05-07 2:56 ` [PATCH 1/2] peel_ref: rename "sha1" argument to "peeled" Jeff King
2013-05-07 3:06 ` [PATCH 2/2] peel_ref: refactor for safety with simultaneous update Jeff King
2013-05-09 19:18 ` [PATCH 3/4] get_packed_refs: reload packed-refs file when it changes Eric Sunshine
2013-05-13 2:43 ` Michael Haggerty
2013-05-07 2:51 ` [PATCH 4/4] for_each_ref: load all loose refs before packed refs Jeff King
2013-05-07 6:40 ` [PATCH 0/4] fix packed-refs races Junio C Hamano
2013-05-07 14:19 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130507023610.GA22053@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johan@herland.net \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).