On 05/07/2013 04:38 AM, Jeff King wrote:
> When we attempt to resolve a ref, the first thing we do is
> call lstat() to see if it is a symlink or a real ref. If we
> find that the ref is missing, we fall back to looking it up
> in the packed-refs file. If we find the loose ref does exist
> (and is a regular file), we continue with trying to open it.
>
> However, we do not do the same fallback if our open() call
> fails; we just report the ref as missing. A "git pack-refs
> --prune" process which is simultaneously running may remove
> the loose ref between our lstat() and open(). In this case,
> we would erroneously report the ref as missing, even though
> we could find it if we checked the packed-refs file.
>
> This patch solves it by factoring out the fallback code from
> the lstat() case and calling it from both places. We do not
> need to do the same thing for the symlink/readlink code
> path, even though it might receive ENOENT, too, because
> symlinks cannot be packed (so if it disappears after the
> lstat, it is truly being deleted).
>
> Note that this solves only the part of the race within
> resolve_ref_unsafe. In the situation described above, we may
> still be depending on a cached view of the packed-refs file;
> that race will be dealt with in a future patch.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
+1, with trivial suggestions below.
> ---
>
> refs.c | 63 ++++++++++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index de2d8eb..5a14703 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1083,6 +1083,43 @@ static int get_packed_ref(const char *refname, unsigned char *sha1)
> return -1;
> }
>
> +/*
> + * This should be called from resolve_ref_unsafe when a loose ref cannot be
> + * accessed; err must represent the errno from the last attempt to access the
> + * loose ref, and the other options are forwarded from resolve_safe_unsaefe.
s/resolve_ref_unsaefe/resolve_ref_unsafe/
> + */
> +static const char *handle_loose_ref_failure(int err,
> + const char *refname,
> + unsigned char *sha1,
> + int reading,
> + int *flag)
> +{
> + /*
> + * If we didn't get ENOENT, something is broken
> + * with the loose ref, and we should not fallback,
> + * but instead pretend it doesn't exist.
> + */
> + if (err != ENOENT)
> + return NULL;
> +
> + /*
> + * The loose reference file does not exist;
> + * check for a packed reference.
> + */
> + if (!get_packed_ref(refname, sha1)) {
> + if (flag)
> + *flag |= REF_ISPACKED;
> + return refname;
> + }
> +
> + /* The reference is not a packed reference, either. */
> + if (reading)
> + return NULL;
> +
> + hashclr(sha1);
> + return refname;
> +}
> +
> const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
> {
> int depth = MAXDEPTH;
> @@ -1107,26 +1144,9 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
>
> git_snpath(path, sizeof(path), "%s", refname);
>
> - if (lstat(path, &st) < 0) {
> - if (errno != ENOENT)
> - return NULL;
> - /*
> - * The loose reference file does not exist;
> - * check for a packed reference.
> - */
> - if (!get_packed_ref(refname, sha1)) {
> - if (flag)
> - *flag |= REF_ISPACKED;
> - return refname;
> - }
> - /* The reference is not a packed reference, either. */
> - if (reading) {
> - return NULL;
> - } else {
> - hashclr(sha1);
> - return refname;
> - }
> - }
> + if (lstat(path, &st) < 0)
> + return handle_loose_ref_failure(errno, refname, sha1,
> + reading, flag);
>
> /* Follow "normalized" - ie "refs/.." symlinks by hand */
> if (S_ISLNK(st.st_mode)) {
> @@ -1156,7 +1176,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
> */
> fd = open(path, O_RDONLY);
> if (fd < 0)
> - return NULL;
> + return handle_loose_ref_failure(errno, refname, sha1,
> + reading, flag);
I probably would have separated the rest of the patch, which is a pure
refactoring, from this last chunk, which is a functional change. But
that's just me.
I suggest adding a comment here mentioning briefly the race condition
that the call to handle_loose_ref_failure() is meant to address;
otherwise somebody not thinking of race conditions might have the clever
idea of "inlining" the call to "return NULL" because it seems redundant
with the check of ENOENT following the lstat() call above.
> len = read_in_full(fd, buffer, sizeof(buffer)-1);
> close(fd);
> if (len < 0)
>
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html