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

From: Jeff King <peff@peff.net>
Date: Mar 11, 2025, 23:06 UTC
Message-ID: <20250311230601.GA72712@coredump.intra.peff.net>
In-Reply-To: <20250311152413.1059343-1-christian.couder@gmail.com>

On Tue, Mar 11, 2025 at 04:24:13PM +0100, Christian Couder wrote:

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

Is a configured remote with out a url key really a missing url, though? In other contexts it defaults to the name of the remote. E.g.:

  # make a repo so "foo" is a valid url
  git init foo
  git -C foo commit --allow-empty bar

  # configure a fetch refspec, but no url!
  git init
  git config remote.foo.fetch '+refs/heads/*:refs/remotes/foo/*'

  # now fetching will use the configured refspec with a url of "foo"
  git fetch foo

  # and git-remote will report it, along with its url
  git remote ;# shows "foo"
  git remote --get-url foo ;# also shows "foo"

This is obviously a weird thing to be doing, so I admit I don't really care all that much. But it feels like the most natural thing is just:

diff --git a/promisor-remote.c b/promisor-remote.c
index 6a0a61382f..761eb1dbd5 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -327,7 +327,7 @@ static void promisor_info_vecs(struct repository *repo,
 		char *url_key = xstrfmt("remote.%s.url", r->name);
 
 		strvec_push(names, r->name);
-		strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url);
+		strvec_push(urls, git_config_get_string(url_key, &url) ? r->name : url);
 
 		free(url);
 		free(url_key);

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

We have xstrdup_or_null() for the latter suggestion. There was some
light discussion at the time about having xstrdup(NULL) handle this
automatically:

  https://lore.kernel.org/git/20150112231231.GA4023@peff.net/

but it was mostly negative. I don't think anybody really dug into the
thought experiment beyond a general "it might propagate NULL places you
wouldn't expect" vibe, though.

For the same reason I'd be a little hesitant to bless NULLs inside
strvec structures. I think "nodup" allowing them is mostly an unintended
consequence.

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

This part I certainly agree with. ;)

>  	for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
> -		char *url;
> +		char *url = NULL;
> +		const char *url_pushed = "";
>  		char *url_key = xstrfmt("remote.%s.url", r->name);
>  
> +		if (!git_config_get_string(url_key, &url) && url)
> +			url_pushed = url;
> +
>  		strvec_push(names, r->name);
> -		strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url);
> +		strvec_push(urls, url_pushed);
>  
>  		free(url);

Probably not super important, but while reading this I noticed that
using git_config_get_string_tmp() would make the memory management a
little simpler (since you do not need to free "url", you are free to
point it to at the empty string and do not need a separate url_pushed).

-Peff