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

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

[Ing. Martin Prantl Ph.D.]

When I try to push to remote git server, I got:

BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL

When I try push to another repository on the same server, it is working correctly. The problem is only with one repo.

Local Git version: 2.36.0.windows.1
Server Git version: 2.19.2

git fsck did not reveal any problem
Checking object directories: 100% (256/256), done.
Checking objects: 100% (23956/23956), done.
Checking connectivity: 18782, done.

git config --list --show-origin
file:C:/Program Files/Git/etc/gitconfig    diff.astextplain.textconv=astextplain
file:C:/Program Files/Git/etc/gitconfig    filter.lfs.clean=git-lfs clean -- %f
file:C:/Program Files/Git/etc/gitconfig    filter.lfs.smudge=git-lfs smudge -- %f
file:C:/Program Files/Git/etc/gitconfig    filter.lfs.process=git-lfs filter-process
file:C:/Program Files/Git/etc/gitconfig    filter.lfs.required=true
file:C:/Program Files/Git/etc/gitconfig    http.sslbackend=openssl
file:C:/Program Files/Git/etc/gitconfig    http.sslcainfo=C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt
file:C:/Program Files/Git/etc/gitconfig    core.autocrlf=true
file:C:/Program Files/Git/etc/gitconfig    core.fscache=true
file:C:/Program Files/Git/etc/gitconfig    core.symlinks=false
file:C:/Program Files/Git/etc/gitconfig    core.editor="C:\\Program Files\\Notepad++\\notepad++.exe" -multiInst -notabbar -nosession -noPlugin
file:C:/Program Files/Git/etc/gitconfig    pull.rebase=false
file:C:/Program Files/Git/etc/gitconfig    credential.helper=manager-core
file:C:/Program Files/Git/etc/gitconfig    credential.https://dev.azure.com.usehttppath=true
file:C:/Program Files/Git/etc/gitconfig    init.defaultbranch=master
file:C:/Users/XXX/.gitconfig    user.name=XXX
file:C:/Users/XXX/.gitconfig    user.email=xxx
file:C:/Users/XXX/.gitconfig    credential.helper=manager-core
file:.git/config    core.bare=false
file:.git/config    core.repositoryformatversion=0
file:.git/config    core.filemode=false
file:.git/config    core.symlinks=false
file:.git/config    core.ignorecase=true
file:.git/config    core.logallrefupdates=true
file:.git/config    remote.origin.url=ssh://xxx/volume1/homes/disk_station/git/repo.git
file:.git/config    remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config    branch..remote=origin
file:.git/config    branch..merge=refs/heads/
file:.git/config    branch.master.remote=origin
file:.git/config    branch.master.merge=refs/heads/master

git ls-remote
BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL

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

[Jeff King]

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.

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

Perhaps something like this, though I'll leave it to the original author
(cc'd) to decide what's best.

diff --git a/remote.c b/remote.c
index 42a4e7106e..2f000a6416 100644
--- a/remote.c
+++ b/remote.c
@@ -354,7 +354,7 @@ 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) {
-		if (!name)
+		if (!name || !namelen)
 			return 0;
 		branch = make_branch(remote_state, name, namelen);
 		if (!strcmp(subkey, "remote")) {

-Peff

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

[Glen Choo]

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

Jeff King <peff@peff.net> writes:

> 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);

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

[Glen Choo]

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

Jeff King <peff@peff.net> writes:

> 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 config that starts with "branch.", 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);