threads / bug / 57896

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

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

## tl;dr

4 messages between May 19, 2022 and May 28, 2022.

replies: 3people: 3as markdown or json

Ing. Martin Prantl Ph.D.· May 19, 2022, 06:23 UTC · lore
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
Jeff King· May 19, 2022, 06:58 UTC · re: Ing. Martin Prantl Ph.D. · lore

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

On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:
Show 6 quoted lines
> 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
Glen Choo· May 28, 2022, 00:12 UTC · re: Jeff King · lore

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

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);
Glen Choo· May 28, 2022, 00:13 UTC · re: Jeff King · lore

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

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

← back to recent threads