Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: May 31, 2017, 09:34 UTC
Message-ID: <CAM0VKjkfuyBzZmpJL47xmK24EOh6X5OjrxtxVdsgGupAseT_wQ@mail.gmail.com>
In-Reply-To: <20170531042304.unkxkuw6s3tcpzkm@sigill.intra.peff.net>

On Wed, May 31, 2017 at 6:23 AM, Jeff King <peff@peff.net> wrote:

Show 30 quoted lines

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

Show 10 quoted lines

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