{"thread":{"id":"57937","subject":"[PATCH 0/2] remote.c: reject 0-length branch names","startedAt":"2022-05-31T23:12:42Z","lastAt":"2022-06-08T23:27:44Z","messageCount":8,"participants":["Glen Choo via GitGitGadget","Ævar Arnfjörð Bjarmason","Glen Choo","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"456425","messageId":"pull.1273.git.git.1654038754.gitgitgadget@gmail.com","threadId":"57937","inReplyTo":null,"subject":"[PATCH 0/2] remote.c: reject 0-length branch names","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-31T23:12:32Z","receivedAt":"2022-05-31T23:12:42Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"This fixes the regression reported in [1], where Git may fail if it\nencounters multiple \"branch.\" keys with no subsection, e.g.\n\n[branch \"\"] remote = foo merge = bar\n\nPrior to 4a2dcb1a08 (remote: die if branch is not found in repository,\n2021-11-17), we silently ignored such keys. But that commit had an\ninconsistency in how we read and write to the branches hash map, causing us\nto BUG() (see Patch 1/2 for more details).\n\nThis small series does two things\n\n * Fix the inconsistency in 4a2dcb1a08 that caused the bug.\n * Make Git fail if we find such keys (because we'll never use them anyway).\n\n[1] https://lore.kernel.org/git/24f547-6285e280-59-40303580@48243747\n\nGlen Choo (2):\n remote.c: don't BUG() on 0-length branch names\n remote.c: reject 0-length branch names\n\n remote.c | 10 ++++++----\n t/t5516-fetch-push.sh | 20 ++++++++++++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\n\nbase-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1273%2Fchooglen%2Fremote%2Fhandle-0-length-branch-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1273/chooglen/remote/handle-0-length-branch-v1\nPull-Request: https://github.com/git/git/pull/1273\n-- \ngitgitgadget\n"},{"id":"456426","messageId":"df6e4db6072e90afc92505a73792cf3c3221d5e4.1654038754.git.gitgitgadget@gmail.com","threadId":"57937","inReplyTo":"pull.1273.git.git.1654038754.gitgitgadget@gmail.com","subject":"[PATCH 1/2] remote.c: don't BUG() on 0-length branch names","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-31T23:12:33Z","receivedAt":"2022-05-31T23:12:44Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\n4a2dcb1a08 (remote: die if branch is not found in repository,\n2021-11-17) introduced a regression where multiple config entries with\nan empty branch name, e.g.\n\n[branch \"\"]\n remote = foo\n merge = bar\n\ncould cause Git to fail when it tries to look up branch tracking\ninformation.\n\nWe parse the config key to get (branch name, branch name length), but\nwhen the branch name subsection is empty, we get a bogus branch name,\ne.g. \"branch..remote\" gives (\".remote\", 0). We continue to use the bogus\nbranch name as if it were valid, and prior to 4a2dcb1a08, this wasn't an\nissue because length = 0 caused the branch name to effectively be \"\"\neverywhere.\n\nHowever, that commit handles length = 0 inconsistently when we create\nthe branch:\n\n- When find_branch() is called to check if the branch exists in the\n branch hash map, it interprets a length of 0 to mean that it should\n call strlen on the char pointer.\n- But the code path that inserts into the branch hash map interprets a\n length of 0 to mean that the string is 0-length.\n\nThis results in the bug described above:\n\n- \"branch..remote\" looks for \".remote\" in the branch hash map. Since we\n do not find it, we insert the \"\" entry into the hash map.\n- \"branch..merge\" looks for \".merge\" in the branch hash map. Since we\n do not find it, we again try to insert the \"\" entry into the hash map.\n However, the entries in the branch hash map are supposed to be\n appended to, not overwritten.\n- Since overwriting an entry is a BUG(), Git fails instead of silently\n ignoring the empty branch name.\n\nFix the bug by removing the convenience strlen functionality, so that\n0 means that the string is 0-length. We still insert a bogus branch name\ninto the hash map, but this will be fixed in a later commit.\n\nReported-by: \"Ing. Martin Prantl Ph.D.\" \nHelped-by: Jeff King \nSigned-off-by: Glen Choo \n---\n remote.c | 6 ++----\n t/t5516-fetch-push.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 42a4e7106e1..cf7015ae8ab 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -195,9 +195,6 @@ static struct branch *find_branch(struct remote_state *remote_state,\n \tstruct branches_hash_key lookup;\n \tstruct hashmap_entry lookup_entry, *e;\n \n-\tif (!len)\n-\t\tlen = strlen(name);\n-\n \tlookup.str = name;\n \tlookup.len = len;\n \thashmap_entry_init(&lookup_entry, memhash(name, len));\n@@ -214,7 +211,8 @@ static void die_on_missing_branch(struct repository *repo,\n {\n \t/* branch == NULL is always valid because it represents detached HEAD. */\n \tif (branch &&\n-\t branch != find_branch(repo->remote_state, branch->name, 0))\n+\t branch != find_branch(repo->remote_state, branch->name,\n+\t\t\t\t strlen(branch->name)))\n \t\tdie(\"branch %s was not found in the repository\", branch->name);\n }\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4dfb080433e..a05268952e9 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -598,6 +598,16 @@ test_expect_success 'branch.*.pushremote config order is irrelevant' '\n \tcheck_push_result two_repo $the_commit heads/main\n '\n \n+test_expect_success 'push ignores empty branch name entries' '\n+\tmk_test one_repo heads/main &&\n+\ttest_config remote.one.url one_repo &&\n+\ttest_config branch..remote one &&\n+\ttest_config branch..merge refs/heads/ &&\n+\ttest_config branch.main.remote one &&\n+\ttest_config branch.main.merge refs/heads/main &&\n+\tgit push\n+'\n+\n test_expect_success 'push with dry-run' '\n \n \tmk_test testrepo heads/main &&\n-- \ngitgitgadget\n\n"},{"id":"456427","messageId":"f947cf221c0b5320d0b7438b88a0d94a5bd3a70b.1654038754.git.gitgitgadget@gmail.com","threadId":"57937","inReplyTo":"pull.1273.git.git.1654038754.gitgitgadget@gmail.com","subject":"[PATCH 2/2] remote.c: reject 0-length branch names","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-31T23:12:34Z","receivedAt":"2022-05-31T23:12:45Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nBranch names can't be empty, so config keys with an empty branch name,\ne.g. \"branch..remote\", are silently ignored.\n\nSince these config keys will never be useful, make it a fatal error when\nremote.c finds a key that starts with \"branch.\" and has an empty\nsubsection.\n\nSigned-off-by: Glen Choo \n---\n remote.c | 4 ++++\n t/t5516-fetch-push.sh | 12 +++++++++++-\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex cf7015ae8ab..a3888dd789c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -352,8 +352,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);\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex a05268952e9..e99c31f8c35 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -598,13 +598,23 @@ test_expect_success 'branch.*.pushremote config order is irrelevant' '\n \tcheck_push_result two_repo $the_commit heads/main\n '\n \n-test_expect_success 'push ignores empty branch name entries' '\n+test_expect_success 'push rejects empty branch name entries' '\n \tmk_test one_repo heads/main &&\n \ttest_config remote.one.url one_repo &&\n \ttest_config branch..remote one &&\n \ttest_config branch..merge refs/heads/ &&\n \ttest_config branch.main.remote one &&\n \ttest_config branch.main.merge refs/heads/main &&\n+\ttest_must_fail git push 2>err &&\n+\tgrep \"bad config variable .branch\\.\\.\" err\n+'\n+\n+test_expect_success 'push ignores \"branch.\" config without subsection' '\n+\tmk_test one_repo heads/main &&\n+\ttest_config remote.one.url one_repo &&\n+\ttest_config branch.autoSetupMerge true &&\n+\ttest_config branch.main.remote one &&\n+\ttest_config branch.main.merge refs/heads/main &&\n \tgit push\n '\n \n-- \ngitgitgadget\n"},{"id":"456437","messageId":"220601.86leug261j.gmgdl@evledraar.gmail.com","threadId":"57937","inReplyTo":"f947cf221c0b5320d0b7438b88a0d94a5bd3a70b.1654038754.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] remote.c: reject 0-length branch names","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T07:30:21Z","receivedAt":"2022-06-01T07:32:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 31 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> Branch names can't be empty, so config keys with an empty branch name,\n> e.g. \"branch..remote\", are silently ignored.\n>\n> Since these config keys will never be useful, make it a fatal error when\n> remote.c finds a key that starts with \"branch.\" and has an empty\n> subsection.\n\nPerhaps this is fine, but I think this commit message (and I checked the\nCL too) really needs to work a bit harder to convince us that this is\nsafe to do.\n\nAre we confident that this is just bizarro config that nobody would have\nhad in practice? In that case I think it's fine to start dying on it.\n\nBut as I understand we previously just ignored this, then if there's any\ndoubt about that perhaps we should start with a warning?\n\nOr are we really confident that this is an edge case not worth worrying\nabout in that way, and that we can go straight to die()?\n"},{"id":"456453","messageId":"kl6lilpke31e.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57937","inReplyTo":"220601.86leug261j.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] remote.c: reject 0-length branch names","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-01T16:55:57Z","receivedAt":"2022-06-01T16:56:07Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, May 31 2022, Glen Choo via GitGitGadget wrote:\n>\n>> From: Glen Choo \n>>\n>> Branch names can't be empty, so config keys with an empty branch name,\n>> e.g. \"branch..remote\", are silently ignored.\n>>\n>> Since these config keys will never be useful, make it a fatal error when\n>> remote.c finds a key that starts with \"branch.\" and has an empty\n>> subsection.\n>\n> Perhaps this is fine, but I think this commit message (and I checked the\n> CL too) really needs to work a bit harder to convince us that this is\n> safe to do.\n\nFair.\n\n> Are we confident that this is just bizarro config that nobody would have\n> had in practice? In that case I think it's fine to start dying on it.\n>\n> But as I understand we previously just ignored this, then if there's any\n> doubt about that perhaps we should start with a warning?\n>\n> Or are we really confident that this is an edge case not worth worrying\n> about in that way, and that we can go straight to die()?\n\nThe case I want to make is even stronger than that - this is an edge\ncase that _we_ shouldn't worry about, and we should tell the _user_ that\ntheir config is bogus.\n\nIt truly makes no sense because `branch..remote` fits the schema of\n`branch..remote` where is \"\", but \"\" isn't a valid branch\nname (and it never has been AFAIK). So such a key would never be useful\nto Git, and it would be extremely hacky for a non-Git tool to use such\na key.\n\nI'm not sure how a user would generate such a key in the wild (e.g.\n[1]). Maybe it was a typo, but more worryingly (I don't have evidence\nfor this, but it could happen), it might be misbehavior from `git\n[branch|config]` that we never noticed because the bogus keys have flown\nunder the radar. If there really is a bug elsewhere, erroring out when\nwe see such keys might also alert us to the bug.\n\nPerhaps I need to capture all of this in the commit message?\n\n[1] https://lore.kernel.org/git/24f547-6285e280-59-40303580@48243747/\n"},{"id":"456470","messageId":"xmqqzgiwozam.fsf@gitster.g","threadId":"57937","inReplyTo":"kl6lilpke31e.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 2/2] remote.c: reject 0-length branch names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-01T21:21:21Z","receivedAt":"2022-06-01T21:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> It truly makes no sense because `branch..remote` fits the schema of\n> `branch..remote` where is \"\", but \"\" isn't a valid branch\n> name (and it never has been AFAIK). So such a key would never be useful\n> to Git, and it would be extremely hacky for a non-Git tool to use such\n> a key.\n\nYup, we might want to reserve a bogus key or two that can never be a\nbranch name to allow us express \"this configuration is in effect for\nall branches\" (e.g. \"branch.*.rebase = never\"), but the natural such\nname would be \"*\" and does not have to be an empty string.\n\n"},{"id":"456907","messageId":"YqEvoB3Bskpykh1w@coredump.intra.peff.net","threadId":"57937","inReplyTo":"kl6lilpke31e.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 2/2] remote.c: reject 0-length branch names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-08T23:24:16Z","receivedAt":"2022-06-08T23:24:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 01, 2022 at 09:55:57AM -0700, Glen Choo wrote:\n\n> > Are we confident that this is just bizarro config that nobody would have\n> > had in practice? In that case I think it's fine to start dying on it.\n> >\n> > But as I understand we previously just ignored this, then if there's any\n> > doubt about that perhaps we should start with a warning?\n> >\n> > Or are we really confident that this is an edge case not worth worrying\n> > about in that way, and that we can go straight to die()?\n> \n> The case I want to make is even stronger than that - this is an edge\n> case that _we_ shouldn't worry about, and we should tell the _user_ that\n> their config is bogus.\n\nIt's a tradeoff, isn't it? We don't know how the user ended up with this\nconfig, what they were trying to do, nor how common it is. Clearly the\nconfig makes no sense and is broken, but by alerting the user, we are:\n\n - maybe doing some good, because now they know that whatever they were\n trying to do didn't work, and can clean up the broken config\n\n - maybe doing some bad, because it was not (and is not) hurting\n anything to have config that nobody bothers to do anything with. But\n if we die, now the user is presented with a situation that they know\n nothing about, and must resolve it before continuing with their\n unrelated work.\n\n-Peff\n"},{"id":"456908","messageId":"YqEwa37mNBqT7M+X@coredump.intra.peff.net","threadId":"57937","inReplyTo":"df6e4db6072e90afc92505a73792cf3c3221d5e4.1654038754.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] remote.c: don't BUG() on 0-length branch names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-08T23:27:39Z","receivedAt":"2022-06-08T23:27:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 31, 2022 at 11:12:33PM +0000, Glen Choo via GitGitGadget wrote:\n\n> Fix the bug by removing the convenience strlen functionality, so that\n> 0 means that the string is 0-length. We still insert a bogus branch name\n> into the hash map, but this will be fixed in a later commit.\n\nI think this is a good change, regardless of whether we take the second\ncommit or not. These kind of \"automagically run strlen() sometimes\"\ninterfaces are subtle, and I think have bitten us before.\n\n> diff --git a/remote.c b/remote.c\n> index 42a4e7106e1..cf7015ae8ab 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -195,9 +195,6 @@ static struct branch *find_branch(struct remote_state *remote_state,\n> \tstruct branches_hash_key lookup;\n> \tstruct hashmap_entry lookup_entry, *e;\n> \n> -\tif (!len)\n> -\t\tlen = strlen(name);\n> -\n> \tlookup.str = name;\n> \tlookup.len = len;\n> \thashmap_entry_init(&lookup_entry, memhash(name, len));\n\nThis changes the behavior of find_branch() without changing its\nsignature. So any topics in flight that use it might be subtly broken. I\nthink that's probably OK in this instance, since it's a file-local\nstatic, and was added relatively recently (i.e., the blast radius is\npretty small and unlikely).\n\n-Peff\n"}]}