Re: [PATCH 1/4] resolve_ref: close race condition for packed refs

> 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@peff.net>

> ---
> 
>  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.

> + */
> +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);