[v3] pull, fetch: fix segfault in --set-upstream option

Commit Message

Ævar Arnfjörð Bjarmason Aug. 30, 2021, 2:41 p.m. UTC

Fix a segfault in the --set-upstream option added in
24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added
in v2.24.0.

The code added there did not do the same checking we do for "git
branch" itself since 8efb8899cfe (branch: segfault fixes and
validation, 2013-02-23), which in turn fixed the same sort of segfault
I'm fixing now in "git branch --set-upstream-to", see
6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).

The warning message I'm adding here is an amalgamation of the error
added for "git branch" in 8efb8899cfe, and the error output
install_branch_config() itself emits, i.e. it trims "refs/heads/" from
the name and says "branch X on remote", not "branch refs/heads/X on
remote".

I think it would make more sense to simply die() here, but in the
other checks for --set-upstream added in 24bc1a12926 we issue a
warning() instead. Let's do the same here for consistency for now.

There was an earlier submitted alternate way of fixing this in [1],
due to that patch breaking threading with the original report at [2] I
didn't notice it before authoring this version. I think the more
detailed warning message here is better, and we should also have tests
for this behavior.

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/

Reported-by: Clemens Fruhwirth <[e-mail]>
Reported-by: Jan Pokorný <[e-mail]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---

A v3 that functionally behaves the same way, but uses a more idiomatic
way of calling skip_prefix() to strip the "refs/heads/*" prefix, if
present.

Range-diff against v2:
1:  9e846b76959 ! 1:  68899471206 pull, fetch: fix segfault in --set-upstream option
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      		}
      		if (source_ref) {
     +			if (!branch) {
    -+				const char *shortname = NULL;
    -+				if (!skip_prefix(source_ref->name,
    -+						 "refs/heads/", &shortname))
    -+					shortname = source_ref->name;
    ++				const char *shortname = source_ref->name;
    ++				skip_prefix(shortname, "refs/heads/", &shortname);
    ++
     +				warning(_("could not set upstream of HEAD to '%s' from '%s' when "
     +					  "it does not point to any branch."),
     +					shortname, transport->remote->name);

 builtin/fetch.c         | 10 ++++++++++
 t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

Comments

Junio C Hamano Aug. 30, 2021, 5:46 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> There was an earlier submitted alternate way of fixing this in [1],
> due to that patch breaking threading with the original report at [2] I
> didn't notice it before authoring this version. I think the more
> detailed warning message here is better, and we should also have tests
> for this behavior.

I do not think it is necessarily an improvement to give more info,
if it is irrelevant to explain what the error is.  And the point of
the error message here is that we cannot set the upstream of
detached HEAD, no matter what the value of old source ref or new
source ref are.

The original from Clemens gives a warning message that omits the
piece of information that does not contribute to the error.

Testing the new behaviour is a good idea.  I also agree with you
that die() would be more appropriate and does not risk regression,
if the original behaviour was to segfault.

Thanks.

Patch

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbdc..28fa168133a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1625,6 +1625,16 @@  static int do_fetch(struct transport *transport,
 			}
 		}
 		if (source_ref) {
+			if (!branch) {
+				const char *shortname = source_ref->name;
+				skip_prefix(shortname, "refs/heads/", &shortname);
+
+				warning(_("could not set upstream of HEAD to '%s' from '%s' when "
+					  "it does not point to any branch."),
+					shortname, transport->remote->name);
+				goto skip;
+			}
+
 			if (!strcmp(source_ref->name, "HEAD") ||
 			    starts_with(source_ref->name, "refs/heads/"))
 				install_branch_config(0,
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index b1d614ce18c..7d12ceff702 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -91,6 +91,17 @@  test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '
 	check_config_missing other2
 '
 
+test_expect_success 'fetch --set-upstream with a detached HEAD' '
+	git checkout HEAD^0 &&
+	test_when_finished "git checkout -" &&
+	cat >expect <<-\EOF &&
+	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
+	EOF
+	git fetch --set-upstream upstream main 2>actual.raw &&
+	grep ^warning: actual.raw >actual &&
+	test_cmp expect actual
+'
+
 # tests for pull --set-upstream
 
 test_expect_success 'setup bare parent pull' '
@@ -178,4 +189,15 @@  test_expect_success 'pull --set-upstream with valid URL and branch sets branch'
 	check_config_missing other2
 '
 
+test_expect_success 'pull --set-upstream with a detached HEAD' '
+	git checkout HEAD^0 &&
+	test_when_finished "git checkout -" &&
+	cat >expect <<-\EOF &&
+	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
+	EOF
+	git pull --set-upstream upstream main 2>actual.raw &&
+	grep ^warning: actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done