{"thread":{"id":"57896","subject":"Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL","startedAt":"2022-05-19T06:29:55Z","lastAt":"2022-05-28T00:13:10Z","messageCount":4,"participants":["Ing. Martin Prantl Ph.D.","Jeff King","Glen Choo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"455451","messageId":"24f547-6285e280-59-40303580@48243747","threadId":"57896","inReplyTo":null,"subject":"Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL","fromName":"Ing. Martin Prantl Ph.D.","fromEmail":"perry@ntis.zcu.cz","sentAt":"2022-05-19T06:23:25Z","receivedAt":"2022-05-19T06:29:55Z","isPatch":false,"sender":{"key":"perry@ntis.zcu.cz","avatar":null},"body":"When I try to push to remote git server, I got:\n\nBUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL\n\nWhen I try push to another repository on the same server, it is working correctly. The problem is only with one repo.\n\nLocal Git version: 2.36.0.windows.1\nServer Git version: 2.19.2\n\ngit fsck did not reveal any problem\nChecking object directories: 100% (256/256), done.\nChecking objects: 100% (23956/23956), done.\nChecking connectivity: 18782, done.\n\ngit config --list --show-origin\nfile:C:/Program Files/Git/etc/gitconfig    diff.astextplain.textconv=astextplain\nfile:C:/Program Files/Git/etc/gitconfig    filter.lfs.clean=git-lfs clean -- %f\nfile:C:/Program Files/Git/etc/gitconfig    filter.lfs.smudge=git-lfs smudge -- %f\nfile:C:/Program Files/Git/etc/gitconfig    filter.lfs.process=git-lfs filter-process\nfile:C:/Program Files/Git/etc/gitconfig    filter.lfs.required=true\nfile:C:/Program Files/Git/etc/gitconfig    http.sslbackend=openssl\nfile:C:/Program Files/Git/etc/gitconfig    http.sslcainfo=C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt\nfile:C:/Program Files/Git/etc/gitconfig    core.autocrlf=true\nfile:C:/Program Files/Git/etc/gitconfig    core.fscache=true\nfile:C:/Program Files/Git/etc/gitconfig    core.symlinks=false\nfile:C:/Program Files/Git/etc/gitconfig    core.editor=\"C:\\\\Program Files\\\\Notepad++\\\\notepad++.exe\" -multiInst -notabbar -nosession -noPlugin\nfile:C:/Program Files/Git/etc/gitconfig    pull.rebase=false\nfile:C:/Program Files/Git/etc/gitconfig    credential.helper=manager-core\nfile:C:/Program Files/Git/etc/gitconfig    credential.https://dev.azure.com.usehttppath=true\nfile:C:/Program Files/Git/etc/gitconfig    init.defaultbranch=master\nfile:C:/Users/XXX/.gitconfig    user.name=XXX\nfile:C:/Users/XXX/.gitconfig    user.email=xxx\nfile:C:/Users/XXX/.gitconfig    credential.helper=manager-core\nfile:.git/config    core.bare=false\nfile:.git/config    core.repositoryformatversion=0\nfile:.git/config    core.filemode=false\nfile:.git/config    core.symlinks=false\nfile:.git/config    core.ignorecase=true\nfile:.git/config    core.logallrefupdates=true\nfile:.git/config    remote.origin.url=ssh://xxx/volume1/homes/disk_station/git/repo.git\nfile:.git/config    remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*\nfile:.git/config    branch..remote=origin\nfile:.git/config    branch..merge=refs/heads/\nfile:.git/config    branch.master.remote=origin\nfile:.git/config    branch.master.merge=refs/heads/master\n\ngit ls-remote\nBUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL\n\n"},{"id":"455452","messageId":"YoXqmrOTxD5MiDU1@coredump.intra.peff.net","threadId":"57896","inReplyTo":"24f547-6285e280-59-40303580@48243747","subject":"Re: Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-05-19T06:58:34Z","receivedAt":"2022-05-19T07:05:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:\n\n> file:.git/config    branch..remote=origin\n> file:.git/config    branch..merge=refs/heads/\n> [...]\n> \n> git ls-remote\n> BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL\n\nThose branch entries with an empty subsection are the culprit. I'm not\nsure how they got there, but they should be safe to remove, which will\nmake your immediate problem go away.\n\nIt looks like handling of such bogus keys regressed in 4a2dcb1a08\n(remote: die if branch is not found in repository, 2021-11-17). In\nmake_branch(), the call to find_branch() gets confused by the 0-length\n\"len\" parameter, and instead uses strlen() on the partial string\ncontaining the rest of the config key. So it tries to look up branch\n\".remote\" for the first key, and \".merge\" for the second. Since neither\nexist, in both cases it then tries to add a new entry, but this time\ncorrectly using the 0-length string. Which will confusingly already be\npresent when handling the second key.\n\nEither find_branch() needs to become more careful about distinguishing\nthe two cases, or perhaps 0-length names should be rejected earlier (I\ndon't think they could ever be useful).\n\nPerhaps something like this, though I'll leave it to the original author\n(cc'd) to decide what's best.\n\ndiff --git a/remote.c b/remote.c\nindex 42a4e7106e..2f000a6416 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -354,7 +354,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \tstruct remote_state *remote_state = cb;\n \n \tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n-\t\tif (!name)\n+\t\tif (!name || !namelen)\n \t\t\treturn 0;\n \t\tbranch = make_branch(remote_state, name, namelen);\n \t\tif (!strcmp(subkey, \"remote\")) {\n\n-Peff\n"},{"id":"456337","messageId":"kl6lh75ams5y.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57896","inReplyTo":"YoXqmrOTxD5MiDU1@coredump.intra.peff.net","subject":"Re: Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-28T00:12:25Z","receivedAt":"2022-05-28T00:12:40Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks for taking a look! (and welcome back :))\n\nJeff King <peff@peff.net> writes:\n\n> On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:\n>\n>> file:.git/config    branch..remote=origin\n>> file:.git/config    branch..merge=refs/heads/\n>> [...]\n>> \n>> git ls-remote\n>> BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL\n>\n> Those branch entries with an empty subsection are the culprit. I'm not\n> sure how they got there, but they should be safe to remove, which will\n> make your immediate problem go away.\n>\n> It looks like handling of such bogus keys regressed in 4a2dcb1a08\n> (remote: die if branch is not found in repository, 2021-11-17). In\n> make_branch(), the call to find_branch() gets confused by the 0-length\n> \"len\" parameter, and instead uses strlen() on the partial string\n> containing the rest of the config key. So it tries to look up branch\n> \".remote\" for the first key, and \".merge\" for the second. Since neither\n> exist, in both cases it then tries to add a new entry, but this time\n> correctly using the 0-length string. Which will confusingly already be\n> present when handling the second key.\n\nIt wasn't obvious to me before what the regression was (since a 0-length\nbranch name is nonsense, right?). Turns out that we used to just ignore\nthe 0-length branch name, but now we BUG(), so yeah this needs fixing.\n\nInterestingly, this 'name=\".remote\" and len=0 confusion' pre-dates that\ncommit, but it got exposed when that commit introduced the confused hash\nmap.\n\nI can get the old behavior by getting rid of the strlen() fallback (I\nthink I will, it doesn't provide any benefit AFAICT), but...\n\n> Either find_branch() needs to become more careful about distinguishing\n> the two cases, or perhaps 0-length names should be rejected earlier (I\n> don't think they could ever be useful).\n\nI think this is even better. Warning the user about their bad config\nsounds like a good thing.\n\nWe would have to be careful not to reject an empty 'name', because this\nmight be a non-subsection branch config, e.g. branch.autoSetupRebase.\nSomething like..\n\ndiff --git a/remote.c b/remote.c\nindex a1463aefb7..d3ae1445a4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -351,8 +351,12 @@ static int handle_config(const char *key, const char *value, void *cb)\n \tstruct remote_state *remote_state = cb;\n \n \tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n+\t\t/* There is no subsection. */\n \t\tif (!name)\n \t\t\treturn 0;\n+\t\t/* There is a subsection, but it is empty. */\n+\t\tif (!namelen)\n+\t\t\treturn -1;\n \t\tbranch = make_branch(remote_state, name, namelen);\n \t\tif (!strcmp(subkey, \"remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n\n"},{"id":"456338","messageId":"kl6lfskums4u.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57896","inReplyTo":"YoXqmrOTxD5MiDU1@coredump.intra.peff.net","subject":"Re: Bug - remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-28T00:13:05Z","receivedAt":"2022-05-28T00:13:10Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Thanks for taking a look! (and welcome back :))\n\nJeff King <peff@peff.net> writes:\n\n> On Thu, May 19, 2022 at 08:23:25AM +0200, Ing. Martin Prantl Ph.D. wrote:\n>\n>> file:.git/config    branch..remote=origin\n>> file:.git/config    branch..merge=refs/heads/\n>> [...]\n>> \n>> git ls-remote\n>> BUG: remote.c:236: hashmap_put overwrote entry after hashmap_get returned NULL\n>\n> Those branch entries with an empty subsection are the culprit. I'm not\n> sure how they got there, but they should be safe to remove, which will\n> make your immediate problem go away.\n>\n> It looks like handling of such bogus keys regressed in 4a2dcb1a08\n> (remote: die if branch is not found in repository, 2021-11-17). In\n> make_branch(), the call to find_branch() gets confused by the 0-length\n> \"len\" parameter, and instead uses strlen() on the partial string\n> containing the rest of the config key. So it tries to look up branch\n> \".remote\" for the first key, and \".merge\" for the second. Since neither\n> exist, in both cases it then tries to add a new entry, but this time\n> correctly using the 0-length string. Which will confusingly already be\n> present when handling the second key.\n\nIt wasn't obvious to me before what the regression was (since a 0-length\nbranch name is nonsense, right?). Turns out that we used to just ignore\nthe 0-length branch name, but now we BUG(), so yeah this needs fixing.\n\nInterestingly, this 'name=\".remote\" and len=0 confusion' pre-dates that\ncommit, but it got exposed when that commit introduced the confused hash\nmap.\n\nI can get the old behavior by getting rid of the strlen() fallback (I\nthink I will, it doesn't provide any benefit AFAICT), but...\n\n> Either find_branch() needs to become more careful about distinguishing\n> the two cases, or perhaps 0-length names should be rejected earlier (I\n> don't think they could ever be useful).\n\nI think this is even better. Warning the user about their bad config\nsounds like a good thing.\n\nWe would have to be careful not to reject an empty 'name', because this\nmight be a non-subsection config that starts with \"branch.\", e.g.\nbranch.autoSetupRebase. Something like..\n\ndiff --git a/remote.c b/remote.c\nindex a1463aefb7..d3ae1445a4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -351,8 +351,12 @@ static int handle_config(const char *key, const char *value, void *cb)\n \tstruct remote_state *remote_state = cb;\n \n \tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n+\t\t/* There is no subsection. */\n \t\tif (!name)\n \t\t\treturn 0;\n+\t\t/* There is a subsection, but it is empty. */\n+\t\tif (!namelen)\n+\t\t\treturn -1;\n \t\tbranch = make_branch(remote_state, name, namelen);\n \t\tif (!strcmp(subkey, \"remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n\n"}]}