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

Commit Message

Glen Choo May 31, 2022, 11:12 p.m. UTC

From: Glen Choo <[e-mail]>

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." <[e-mail]>
Helped-by: Jeff King <[e-mail]>
Signed-off-by: Glen Choo <[e-mail]>
---
 remote.c              |  6 ++----
 t/t5516-fetch-push.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Jeff King June 8, 2022, 11:27 p.m. UTC | #1

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

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 &&