On Wed, May 31, 2017 at 11:34:23AM +0200, SZEDER Gábor wrote:
> >> +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.
Yeah, I agree it is safe now. I'm just worried about some function in
remote.c later doing:
read_config();
add_and_parse_fetch_refspec(remotes[0], whatever);
which leaves the struct in an inconsistent state (we realloc NULL which
allocates from scratch, and all of the other entries in remote->fetch
end up uninitialized). Can we at least add an assertion like:
if (!remote->fetch)
BUG("cannot add refspec to an unparsed remote");
?
-Peff