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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2025, 16:29 UTC
Message-ID: <xmqqo6y897cz.fsf@gitster.g>
In-Reply-To: <20250310074053.1886097-1-christian.couder@gmail.com>

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

> Pushing NULL into a 'strvec' results in a segfault, as 'strvec' is a
> kind of NULL terminated array that is designed to be compatible with
> 'argv' variables used on the command line.

It is good that you corrected a caller that tries to make a strvec into such a state, but shouldn't strvec_push() protect itself with a BUG or something, I have to wonder.

> So when an URL is missing from the config, let's push an empty string
> instead of NULL into the 'strvec' that stores URLs.

How will these strings in the "names" strvec used? When URLs are present, I'm sure we are going to use it as URL, but when they are missing, what happens? The second hunk of this patch seems to ignore such a broken entry with an empty URL, but that smells like sweeping a problem under the rug. Shouldn't such a promisor be either diagnosed as an error, or skipped when you accumulate URLs to be used into the strvec, so that the later code (i.e. the second hunk) does not even have to worry about it?

Show 6 quoted lines

> @@ -356,7 +360,7 @@ char *promisor_remote_info(struct repository *repo)
>  			strbuf_addch(&sb, ';');
>  		strbuf_addstr(&sb, "name=");
>  		strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
> -		if (urls.v[i]) {
> +		if (urls.v[i] && *urls.v[i]) {

Why does urls.v[i] need to be checked? Didn't you just make sure that the strvec would not have NULL in it?