Re: [PATCH v2] promisor-remote: fix segfault when remote URL is missing

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> Using strvec_push() to push `NULL` into a 'strvec' results in a
> segfault, because `xstrdup(NULL)` crashes.
>
> So when an URL is missing from the config, let's push an empty string
> instead of `NULL` into the 'strvec' that stores URLs.
>

> We could have modified strvec_push() to behave like
> strvec_push_nodup() and accept `NULL`, but it's not clear that it's
> the right thing to do for the strvec API. 'strvec' is a kind of NULL
> terminated array that is designed to be compatible with 'argv'
> variables used on the command line. So we might want to disallow
> pushing any `NULL` in it instead.
>
> It's also not clear if `xstrdup(NULL)` should crash or BUG or just
> return NULL.

Yup, the above two paragraphs are irrelevant, I would think.

What we could have done may be to ignore (or error out) a
configuration entry that lacks URL as an error.  After all, isn't
this caused by a misconfiguration?  How such a misconfiguration is
swept under the rug, whether with an empty string or NULL, is
secondary, I would think.

> For all these reasons, let's just focus on fixing the issue in
> "promisor-remote.c" and let's leave improving the strvec API and/or
> xstrdup() for a future effort.

Absolutely.

> While at it let's warn and reject the remote, in the 'KnownUrl'
> case, when no URL is advertised by the server or no URL is
> configured on the client for a remote name advertised by the server
> and configured on the client. This is on par with a warning already
> emitted when URLs are different in the same case.

Yup.