patch, 2 partsAdd test for ls-tree with broken symlink under refs/heads

> From: Michael Haggerty <mhagger@alum.mit.edu>
> 
> If there is a broken symlink where a loose reference file is expected,
> then the attempt to open() it fails with ENOENT. This error is
> misinterpreted to mean that the loose reference file itself has
> disappeared due to a race, causing the lookup to be retried. But in
> this scenario, the retries all suffer from the same problem, causing
> an infinite loop.
> 
> So put a limit (of 5) on the number of times that the stat_ref step
> can be retried.
> 
> Based-on-patch-by: Petr Stodulka <pstodulk@redhat.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs/files-backend.c | 6 ++++--
>  refs/refs-internal.h | 6 ++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d16feb1..245a0b5 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	int fd;
>  	int ret = -1;
>  	int save_errno;
> +	int retries = 0;
>  
>  	*type = 0;
>  	strbuf_reset(&sb_path);
> @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	if (S_ISLNK(st.st_mode)) {
>  		strbuf_reset(&sb_contents);
>  		if (strbuf_readlink(&sb_contents, path, 0) < 0) {
> -			if (errno == ENOENT || errno == EINVAL)
> +			if ((errno == ENOENT || errno == EINVAL) &&
> +				retries++ < MAXRETRIES) 
>  				/* inconsistent with lstat; retry */
>  				goto stat_ref;
>  			else
> @@ -1426,7 +1428,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	 */
>  	fd = open(path, O_RDONLY);
>  	if (fd < 0) {
> -		if (errno == ENOENT)
> +		if (errno == ENOENT && retries++ < MAXRETRIES)
>  			/* inconsistent with lstat; retry */
>  			goto stat_ref;
>  		else
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 708b260..37e6b99 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const char *new_refname);
>  /* We allow "recursive" symbolic refs. Only within reason, though */
>  #define SYMREF_MAXDEPTH 5
>  
> +/* 
> + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible
> + * infinite loop
> + */
> +#define MAXRETRIES 5
> +
>  /* Include broken references in a do_for_each_ref*() iteration: */
>  #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
>  
> 

> I have realized that this wasn't fixed after all when refs.c
> was "rewritten". Issue is caused by broken symlink under refs/heads,
> which causes infinite loop for "git ls-tree" command. It was replied
> earlier [0] and Michael previously fixed that in better way probably,
> then my proposed patch 2/2, but it contains more changes and I have not
> enough time to study changes in refs* code, so I propose now just my
> little patch, which was previously modified by Michael.
> 
> If you prefer different solution, I am ok with this. It is really
> just quick proposal. Patch 1/2 contains test for this issue. If you
> will prefer different solution with different exit code, test should
> be corrected. Basic idea is, that timeout should'n expire with
> exit code 124.
> 
> [0] http://marc.info/?l=git&m=142712669103790&w=2
> [1] https://github.com/mhagger/git, branch "ref-broken-symlinks"

> On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote:
> 
>> I have realized that this wasn't fixed after all when refs.c
>> was "rewritten". Issue is caused by broken symlink under refs/heads,
>> which causes infinite loop for "git ls-tree" command. It was replied
>> earlier [0] and Michael previously fixed that in better way probably,
>> then my proposed patch 2/2, but it contains more changes and I have not
>> enough time to study changes in refs* code, so I propose now just my
>> little patch, which was previously modified by Michael.
>>
>> If you prefer different solution, I am ok with this. It is really
>> just quick proposal. Patch 1/2 contains test for this issue. If you
>> will prefer different solution with different exit code, test should
>> be corrected. Basic idea is, that timeout should'n expire with
>> exit code 124.
>>
>> [0] http://marc.info/?l=git&m=142712669103790&w=2
>> [1] https://github.com/mhagger/git, branch "ref-broken-symlinks"
> 
> I think I fixed this semi-independently last week; see the thread at:
> 
>   http://public-inbox.org/git/20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net/
> 
> I say semi-independently because I didn't actually know about your
> previous report, but saw it in the wild myself. But I did talk with
> Michael off-list, and he suggested the belt-and-suspenders retry counter
> in my second patch.
> 
> The fix is at e8c42cb in Junio's tree, and it's currently merged to
> 'next'.
> 
> -Peff
>