Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
>>> index c180e0aad7..e5e5df16d8 100644
>>> --- a/refs/ref-cache.c
>>> +++ b/refs/ref-cache.c
>>> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>>>  				 */
>>>  				break;
>>>  			}
>>> -		} while (slash);
>>> +		} while (slash && dir->nr);
>>>  	}
>>
>> This is at the tail of a "do { ... } while (...);" loop, but inside
>> the loop I see this construct:
>>
>> 			for (idx = 0; idx < dir->nr; idx++) {
>> 				cmp = strncmp(refname, dir->entries[idx]->name, len);
>> 				if (cmp <= 0)
>> 					break;
>> 			}
>> 			/* don't overflow the index */
>> 			idx = idx >= dir->nr ? dir->nr - 1 : idx;
>>
>> i.e., if we scan all the dir->entries[] elements in the innter loop
>> and did not find any hit, idx would become dir->nr and this inner
>> loop runs to the end.  If (dir->nr == 0), then ?: operator [*] would
>> become the idx = (dir->nr - 1);  And that idx is used for a while
>> before we get to this "while (slash && dir->nr)".
>>
>
> This wouldn't happen because before the loop starts, we set:
>
>    dir = get_ref_dir(iter->cache->root);
>
> So, `dir` will always have an entry (i.e. 'refs') in the first
> iteration, after that, the checks in the while section  of the 'do {}
> while (...)' loop will kick in.
>
>> And then tha tis used like this.
>>
>> 			if (slash)
>> 				slash = slash + 1;
>>
>> 			level->index = idx;
>> 			if (dir->entries[idx]->flag & REF_DIR) {
>> 				...
>>
>> IOW, isn't this check a bit too late?  I wonder if we can leave at
>> the beginning of the outer loop, even before sort_ref_dir(dir), when
>> dir->nr is zero, or something?
>>
>>
>
> We could add it there too. I can't see the merit of one over the other.
> But if you see it being more readable. I'll happily make that change.
>