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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2025, 16:59 UTC
Message-ID: <xmqqikof4i60.fsf@gitster.g>
In-Reply-To: <20250311152413.1059343-1-christian.couder@gmail.com>

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

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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