On Wed, May 31, 2017 at 6:23 AM, Jeff King <peff@xxxxxxxx> wrote:
> On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:
>
>> diff --git a/remote.c b/remote.c
>> index ad6c5424e..b8fd09dc9 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>> return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>> }
>>
>> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
>> +{
>> + struct refspec *rs;
>> +
>> + add_fetch_refspec(remote, refspec);
>> + rs = parse_fetch_refspec(1, &refspec);
>> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
>> + remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
>> +
>> + /* Not free_refspecs(), as we copied its pointers above */
>> + free(rs);
>> +}
>
> What happens here if remote->fetch isn't already initialized? I think
> we'd end up with a bunch of garbage values. That's what I was trying to
> protect against in my original suggestion.
>
> I'm not sure if that's possible or not. We seem to initialize it in both
> remote_get() and for_each_remote(), and I don't think there are any
> other ways to get a remote.
The only place creating remotes is remote.c:make_remote(), which
calloc()s the required memory, making all of struct remote's fields
zero-initialized. In case of clone the common case is that the user
doesn't specify any additional fetch refspecs, so remote->fetch will
still be NULL after full initialization and when
add_and_parse_fetch_refspec() is called with the default fetch
refspec, meaning we can't 'if (remote->fetch) { parse ... }'. OTOH,
all functions involved can cope with the fetch-refspec-related fields
being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for
array indexing it's not 0 anymore.
> (In fact, I kind of wondered why we do this
> parsing lazily at all, but I think it's so that misconfigured remotes
> don't cause us to die() if we aren't accessing them at all).
>
> If we assume that the contract that remote.c provides is that the
> fetch fields are always filled in before a "struct remote" is returned
> to a caller, and that only such callers would use this function, it's
> OK. It just seems more dangerous than it needs to be.
>
> -Peff