Re: Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL

From: Glen Choo <chooglen@google.com>
Date: May 28, 2022, 00:12 UTC
Message-ID: <kl6lh75ams5y.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <YoXqmrOTxD5MiDU1@coredump.intra.peff.net>

Thanks for taking a look! (and welcome back :))

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:
>
>> file:.git/config    branch..remote=origin
>> file:.git/config    branch..merge=refs/heads/
>> [...]
>> 
>> git ls-remote
>> BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL
>
> Those branch entries with an empty subsection are the culprit. I'm not
> sure how they got there, but they should be safe to remove, which will
> make your immediate problem go away.
>
> It looks like handling of such bogus keys regressed in 4a2dcb1a08
> (remote: die if branch is not found in repository, 2021-11-17). In
> make_branch(), the call to find_branch() gets confused by the 0-length
> "len" parameter, and instead uses strlen() on the partial string
> containing the rest of the config key. So it tries to look up branch
> ".remote" for the first key, and ".merge" for the second. Since neither
> exist, in both cases it then tries to add a new entry, but this time
> correctly using the 0-length string. Which will confusingly already be
> present when handling the second key.

It wasn't obvious to me before what the regression was (since a 0-length branch name is nonsense, right?). Turns out that we used to just ignore the 0-length branch name, but now we BUG(), so yeah this needs fixing.

Interestingly, this 'name=".remote" and len=0 confusion' pre-dates that commit, but it got exposed when that commit introduced the confused hash map.

I can get the old behavior by getting rid of the strlen() fallback (I think I will, it doesn't provide any benefit AFAICT), but...

> Either find_branch() needs to become more careful about distinguishing
> the two cases, or perhaps 0-length names should be rejected earlier (I
> don't think they could ever be useful).

I think this is even better. Warning the user about their bad config sounds like a good thing.

We would have to be careful not to reject an empty 'name', because this might be a non-subsection branch config, e.g. branch.autoSetupRebase. Something like..

diff --git a/remote.c b/remote.c
index a1463aefb7..d3ae1445a4 100644
--- a/remote.c
+++ b/remote.c
@@ -351,8 +351,12 @@ static int handle_config(const char *key, const char *value, void *cb)
 	struct remote_state *remote_state = cb;
 
 	if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
+		/* There is no subsection. */
 		if (!name)
 			return 0;
+		/* There is a subsection, but it is empty. */
+		if (!namelen)
+			return -1;
 		branch = make_branch(remote_state, name, namelen);
 		if (!strcmp(subkey, "remote")) {
 			return git_config_string(&branch->remote_name, key, value);