[PATCH 0/2] remote.c: reject 0-length branch names

[PATCH 0/2] remote.c: reject 0-length branch names

[Glen Choo via GitGitGadget]

This fixes the regression reported in [1], where Git may fail if it
encounters multiple "branch." keys with no subsection, e.g.

[branch ""] remote = foo merge = bar

Prior to 4a2dcb1a08 (remote: die if branch is not found in repository,
2021-11-17), we silently ignored such keys. But that commit had an
inconsistency in how we read and write to the branches hash map, causing us
to BUG() (see Patch 1/2 for more details).

This small series does two things

 * Fix the inconsistency in 4a2dcb1a08 that caused the bug.
 * Make Git fail if we find such keys (because we'll never use them anyway).

[1] https://lore.kernel.org/git/24f547-6285e280-59-40303580@48243747

Glen Choo (2):
  remote.c: don't BUG() on 0-length branch names
  remote.c: reject 0-length branch names

 remote.c              | 10 ++++++----
 t/t5516-fetch-push.sh | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)


base-commit: f9b95943b68b6b8ca5a6072f50a08411c6449b55
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1273%2Fchooglen%2Fremote%2Fhandle-0-length-branch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1273/chooglen/remote/handle-0-length-branch-v1
Pull-Request: https://github.com/git/git/pull/1273
-- 
gitgitgadget

[PATCH 1/2] remote.c: don't BUG() on 0-length branch names

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

4a2dcb1a08 (remote: die if branch is not found in repository,
2021-11-17) introduced a regression where multiple config entries with
an empty branch name, e.g.

[branch ""]
  remote = foo
  merge = bar

could cause Git to fail when it tries to look up branch tracking
information.

We parse the config key to get (branch name, branch name length), but
when the branch name subsection is empty, we get a bogus branch name,
e.g. "branch..remote" gives (".remote", 0). We continue to use the bogus
branch name as if it were valid, and prior to 4a2dcb1a08, this wasn't an
issue because length = 0 caused the branch name to effectively be ""
everywhere.

However, that commit handles length = 0 inconsistently when we create
the branch:

- When find_branch() is called to check if the branch exists in the
  branch hash map, it interprets a length of 0 to mean that it should
  call strlen on the char pointer.
- But the code path that inserts into the branch hash map interprets a
  length of 0 to mean that the string is 0-length.

This results in the bug described above:

- "branch..remote" looks for ".remote" in the branch hash map. Since we
  do not find it, we insert the "" entry into the hash map.
- "branch..merge" looks for ".merge" in the branch hash map. Since we
  do not find it, we again try to insert the "" entry into the hash map.
  However, the entries in the branch hash map are supposed to be
  appended to, not overwritten.
- Since overwriting an entry is a BUG(), Git fails instead of silently
  ignoring the empty branch name.

Fix the bug by removing the convenience strlen functionality, so that
0 means that the string is 0-length. We still insert a bogus branch name
into the hash map, but this will be fixed in a later commit.

Reported-by: "Ing. Martin Prantl Ph.D." <perry@ntis.zcu.cz>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c              |  6 ++----
 t/t5516-fetch-push.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 42a4e7106e1..cf7015ae8ab 100644
--- a/remote.c
+++ b/remote.c
@@ -195,9 +195,6 @@ static struct branch *find_branch(struct remote_state *remote_state,
 	struct branches_hash_key lookup;
 	struct hashmap_entry lookup_entry, *e;
 
-	if (!len)
-		len = strlen(name);
-
 	lookup.str = name;
 	lookup.len = len;
 	hashmap_entry_init(&lookup_entry, memhash(name, len));
@@ -214,7 +211,8 @@ static void die_on_missing_branch(struct repository *repo,
 {
 	/* branch == NULL is always valid because it represents detached HEAD. */
 	if (branch &&
-	    branch != find_branch(repo->remote_state, branch->name, 0))
+	    branch != find_branch(repo->remote_state, branch->name,
+				  strlen(branch->name)))
 		die("branch %s was not found in the repository", branch->name);
 }
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4dfb080433e..a05268952e9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -598,6 +598,16 @@ test_expect_success 'branch.*.pushremote config order is irrelevant' '
 	check_push_result two_repo $the_commit heads/main
 '
 
+test_expect_success 'push ignores empty branch name entries' '
+	mk_test one_repo heads/main &&
+	test_config remote.one.url one_repo &&
+	test_config branch..remote one &&
+	test_config branch..merge refs/heads/ &&
+	test_config branch.main.remote one &&
+	test_config branch.main.merge refs/heads/main &&
+	git push
+'
+
 test_expect_success 'push with dry-run' '
 
 	mk_test testrepo heads/main &&
-- 
gitgitgadget

Re: [PATCH 1/2] remote.c: don't BUG() on 0-length branch names

[Jeff King]

On Tue, May 31, 2022 at 11:12:33PM +0000, Glen Choo via GitGitGadget wrote:

> Fix the bug by removing the convenience strlen functionality, so that
> 0 means that the string is 0-length. We still insert a bogus branch name
> into the hash map, but this will be fixed in a later commit.

I think this is a good change, regardless of whether we take the second
commit or not. These kind of "automagically run strlen() sometimes"
interfaces are subtle, and I think have bitten us before.

> diff --git a/remote.c b/remote.c
> index 42a4e7106e1..cf7015ae8ab 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -195,9 +195,6 @@ static struct branch *find_branch(struct remote_state *remote_state,
>  	struct branches_hash_key lookup;
>  	struct hashmap_entry lookup_entry, *e;
>  
> -	if (!len)
> -		len = strlen(name);
> -
>  	lookup.str = name;
>  	lookup.len = len;
>  	hashmap_entry_init(&lookup_entry, memhash(name, len));

This changes the behavior of find_branch() without changing its
signature. So any topics in flight that use it might be subtly broken. I
think that's probably OK in this instance, since it's a file-local
static, and was added relatively recently (i.e., the blast radius is
pretty small and unlikely).

-Peff

[PATCH 2/2] remote.c: reject 0-length branch names

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

Branch names can't be empty, so config keys with an empty branch name,
e.g. "branch..remote", are silently ignored.

Since these config keys will never be useful, make it a fatal error when
remote.c finds a key that starts with "branch." and has an empty
subsection.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 remote.c              |  4 ++++
 t/t5516-fetch-push.sh | 12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index cf7015ae8ab..a3888dd789c 100644
--- a/remote.c
+++ b/remote.c
@@ -352,8 +352,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);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index a05268952e9..e99c31f8c35 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -598,13 +598,23 @@ test_expect_success 'branch.*.pushremote config order is irrelevant' '
 	check_push_result two_repo $the_commit heads/main
 '
 
-test_expect_success 'push ignores empty branch name entries' '
+test_expect_success 'push rejects empty branch name entries' '
 	mk_test one_repo heads/main &&
 	test_config remote.one.url one_repo &&
 	test_config branch..remote one &&
 	test_config branch..merge refs/heads/ &&
 	test_config branch.main.remote one &&
 	test_config branch.main.merge refs/heads/main &&
+	test_must_fail git push 2>err &&
+	grep "bad config variable .branch\.\." err
+'
+
+test_expect_success 'push ignores "branch." config without subsection' '
+	mk_test one_repo heads/main &&
+	test_config remote.one.url one_repo &&
+	test_config branch.autoSetupMerge true &&
+	test_config branch.main.remote one &&
+	test_config branch.main.merge refs/heads/main &&
 	git push
 '
 
-- 
gitgitgadget

Re: [PATCH 2/2] remote.c: reject 0-length branch names

[Ævar Arnfjörð Bjarmason]

On Tue, May 31 2022, Glen Choo via GitGitGadget wrote:

> From: Glen Choo <chooglen@google.com>
>
> Branch names can't be empty, so config keys with an empty branch name,
> e.g. "branch..remote", are silently ignored.
>
> Since these config keys will never be useful, make it a fatal error when
> remote.c finds a key that starts with "branch." and has an empty
> subsection.

Perhaps this is fine, but I think this commit message (and I checked the
CL too) really needs to work a bit harder to convince us that this is
safe to do.

Are we confident that this is just bizarro config that nobody would have
had in practice? In that case I think it's fine to start dying on it.

But as I understand we previously just ignored this, then if there's any
doubt about that perhaps we should start with a warning?

Or are we really confident that this is an edge case not worth worrying
about in that way, and that we can go straight to die()?

Re: [PATCH 2/2] remote.c: reject 0-length branch names

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, May 31 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@google.com>
>>
>> Branch names can't be empty, so config keys with an empty branch name,
>> e.g. "branch..remote", are silently ignored.
>>
>> Since these config keys will never be useful, make it a fatal error when
>> remote.c finds a key that starts with "branch." and has an empty
>> subsection.
>
> Perhaps this is fine, but I think this commit message (and I checked the
> CL too) really needs to work a bit harder to convince us that this is
> safe to do.

Fair.

> Are we confident that this is just bizarro config that nobody would have
> had in practice? In that case I think it's fine to start dying on it.
>
> But as I understand we previously just ignored this, then if there's any
> doubt about that perhaps we should start with a warning?
>
> Or are we really confident that this is an edge case not worth worrying
> about in that way, and that we can go straight to die()?

The case I want to make is even stronger than that - this is an edge
case that _we_ shouldn't worry about, and we should tell the _user_ that
their config is bogus.

It truly makes no sense because `branch..remote` fits the schema of
`branch.<name>.remote` where <name> is "", but "" isn't a valid branch
name (and it never has been AFAIK). So such a key would never be useful
to Git, and it would be extremely hacky for a non-Git tool to use such
a key.

I'm not sure how a user would generate such a key in the wild (e.g.
[1]). Maybe it was a typo, but more worryingly (I don't have evidence
for this, but it could happen), it might be misbehavior from `git
[branch|config]` that we never noticed because the bogus keys have flown
under the radar. If there really is a bug elsewhere, erroring out when
we see such keys might also alert us to the bug.

Perhaps I need to capture all of this in the commit message?

[1] https://lore.kernel.org/git/24f547-6285e280-59-40303580@48243747/

Re: [PATCH 2/2] remote.c: reject 0-length branch names

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

> It truly makes no sense because `branch..remote` fits the schema of
> `branch.<name>.remote` where <name> is "", but "" isn't a valid branch
> name (and it never has been AFAIK). So such a key would never be useful
> to Git, and it would be extremely hacky for a non-Git tool to use such
> a key.

Yup, we might want to reserve a bogus key or two that can never be a
branch name to allow us express "this configuration is in effect for
all branches" (e.g. "branch.*.rebase = never"), but the natural such
name would be "*" and does not have to be an empty string.

Re: [PATCH 2/2] remote.c: reject 0-length branch names

[Jeff King]

On Wed, Jun 01, 2022 at 09:55:57AM -0700, Glen Choo wrote:

> > Are we confident that this is just bizarro config that nobody would have
> > had in practice? In that case I think it's fine to start dying on it.
> >
> > But as I understand we previously just ignored this, then if there's any
> > doubt about that perhaps we should start with a warning?
> >
> > Or are we really confident that this is an edge case not worth worrying
> > about in that way, and that we can go straight to die()?
> 
> The case I want to make is even stronger than that - this is an edge
> case that _we_ shouldn't worry about, and we should tell the _user_ that
> their config is bogus.

It's a tradeoff, isn't it? We don't know how the user ended up with this
config, what they were trying to do, nor how common it is. Clearly the
config makes no sense and is broken, but by alerting the user, we are:

  - maybe doing some good, because now they know that whatever they were
    trying to do didn't work, and can clean up the broken config

  - maybe doing some bad, because it was not (and is not) hurting
    anything to have config that nobody bothers to do anything with. But
    if we die, now the user is presented with a situation that they know
    nothing about, and must resolve it before continuing with their
    unrelated work.

-Peff