git pull --set-upstream segfaults on branchless repo.

git pull --set-upstream segfaults on branchless repo.

[Clemens Fruhwirth]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

Run "git pull origin nixos-unstable --rebase --set-upstream"
on a repo that had no branch set, e.g. when running "git branch" gave
"* (no branch)"

What did you expect to happen? (Expected behavior)
Pull from upstream and set upstream.

What happened instead? (Actual behavior)
Segfault with the following trace:
(gdb) bt
#0  0x000000000044a8c9 in do_fetch (transport=0x2557920,
rs=rs@entry=0x7ffd42335c00) at builtin/fetch.c:1568
#1  0x000000000044ac61 in fetch_one (remote=<optimized out>,
argc=1110662144, argv=0x7ffd42335fc8,
    prune_tags_ok=<optimized out>, use_stdin_refspecs=0) at builtin/fetch.c:1892
#2  0x000000000044af15 in cmd_fetch (argc=0, argv=0x7ffd42335fc8,
prefix=0x0) at builtin/fetch.c:1992
#3  0x0000000000406354 in run_builtin (p=0x70d3e0 <commands+960>,
argc=argc@entry=5, argv=argv@entry=0x7ffd42335fc0)
    at git.c:453
#4  0x00000000004065c3 in handle_builtin (argc=5, argv=0x7ffd42335fc0)
at git.c:704
#5  0x0000000000407c4a in run_argv (argcp=argcp@entry=0x7ffd42335e8c,
argv=argv@entry=0x7ffd42335e80) at git.c:771
#6  0x00000000004080a4 in cmd_main (argc=<optimized out>,
argc@entry=6, argv=<optimized out>,
    argv@entry=0x7ffd42335fb8) at git.c:902
#7  0x00000000004c614c in main (argc=6, argv=0x7ffd42335fb8) at common-main.c:52
(gdb) p branch
$1 = (struct branch *) 0x0

What's different between what you expected and what actually happened?
Not segfault

Anything else you want to add:

Dropping the "--set-upstream" makes the segfault go away. Looking at
builtin/fetch.c around L1568 I see,

        if (set_upstream) {
                struct branch *branch = branch_get("HEAD");
                [..]
                if (source_ref) {
                        if (!strcmp(source_ref->name, "HEAD") ||
                            starts_with(source_ref->name, "refs/heads/"))
                                install_branch_config(0,
                                                      branch->name, //
<- SEGFAULT HERE
                                                      transport->remote->name,
                                                      source_ref->name);
                        [..]
              }
        }

It's rather clear that branch is just NULL from the gdb session above,
and the branch->name dereference fails.
It might be useful to catch branch == NULL.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.31.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /nix/store/kxj6cblcsd1qcbbxlmbswwrn89zcmgd6-bash-4.4-p23/bin/bash
uname: Linux 5.12.12 #1-NixOS SMP Fri Jun 18 08:02:52 UTC 2021 x86_64
compiler info: gnuc: 10.3
libc info: glibc: 2.32
$SHELL (typically, interactive shell): /var/run/current-system/sw/bin/zsh


[Enabled Hooks]

(Please cc me on replies, not subscribe to the mailing list).
-- 
Fruhwirth Clemens http://clemens.endorphin.org

Re: git pull --set-upstream segfaults on branchless repo.

[Jan Pokorný]

Hello,

Clemens Fruhwirth píše v Po 05. 07. 2021 v 17:46 +0200:
> What did you do before the bug happened? (Steps to reproduce your
> issue)
> 
> Run "git pull origin nixos-unstable --rebase --set-upstream"
> on a repo that had no branch set, e.g. when running "git branch" gave
> "* (no branch)"
> 
> What did you expect to happen? (Expected behavior)
> Pull from upstream and set upstream.
> 
> What happened instead? (Actual behavior)
> Segfault with the following trace:
> (gdb) bt
> #0  0x000000000044a8c9 in do_fetch (transport=0x2557920, rs=rs@entry=0x7ffd42335c00) at builtin/fetch.c:1568
> #1  0x000000000044ac61 in fetch_one (remote=<optimized out>, argc=1110662144, argv=0x7ffd42335fc8, prune_tags_ok=<optimized out>, use_stdin_refspecs=0) at builtin/fetch.c:1892
> #2  0x000000000044af15 in cmd_fetch (argc=0, argv=0x7ffd42335fc8, prefix=0x0) at builtin/fetch.c:1992
> #3  0x0000000000406354 in run_builtin (p=0x70d3e0 <commands+960>, argc=argc@entry=5, argv=argv@entry=0x7ffd42335fc0) at git.c:453
> #4  0x00000000004065c3 in handle_builtin (argc=5, argv=0x7ffd42335fc0) at git.c:704
> #5  0x0000000000407c4a in run_argv (argcp=argcp@entry=0x7ffd42335e8c, argv=argv@entry=0x7ffd42335e80) at git.c:771
> #6  0x00000000004080a4 in cmd_main (argc=<optimized out>, argc@entry=6, argv=<optimized out>, argv@entry=0x7ffd42335fb8) at git.c:902
> #7  0x00000000004c614c in main (argc=6, argv=0x7ffd42335fb8) at common-main.c:52
> (gdb) p branch
> $1 = (struct branch *) 0x0
> 
> What's different between what you expected and what actually happened?
> Not segfault
> 
> Anything else you want to add:
> 
> Dropping the "--set-upstream" makes the segfault go away. Looking at
> builtin/fetch.c around L1568 I see,
> 
>         if (set_upstream) {
>                 struct branch *branch = branch_get("HEAD");
>                 [..]
>                 if (source_ref) {
>                         if (!strcmp(source_ref->name, "HEAD") ||
>                             starts_with(source_ref->name, "refs/heads/"))
>                                 install_branch_config(0,
>                                                       branch->name, // <- SEGFAULT HERE
>                                                       transport->remote->name,
>                                                       source_ref->name);
>                         [..]
>               }
>         }
> 
> It's rather clear that branch is just NULL from the gdb session above,
> and the branch->name dereference fails.
> It might be useful to catch branch == NULL.
> 
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.
> 
> [System Info]
> git version:
> git version 2.31.1
> [...]

observed something pretty similar on Fedora, with both
git-2.32.0-0.3.rc2.fc35.x86_64 and git-2.32.0-1.fc35.x86_64, e.g.:

$ git clone https://github.com/git/git.git
[...]
$ cd git
$ git checkout --detach v2.1.0
HEAD is now at 6c4ab27f23 Git 2.1
$ git pull --set-upstream origin master
From https://github.com/git/git
 * branch                  master     -> FETCH_HEAD
error: fetch died of signal 11

  1625			if (source_ref) {
  1626				if (!strcmp(source_ref->name, "HEAD") ||
  1627				   starts_with(source_ref->name, "refs/heads/"))
=>1628					install_branch_config(0,
  1629							     branch->name,
  1630							     transport->remote->name,
  1631							     source_ref->name);

(gdb) p branch
$1 = <optimized out>
(gdb) p branch->name
value has been optimized out

   0x000055b5bceba921 <+8705>:	nopl   0x0(%rax)
   0x000055b5bceba928 <+8712>:	mov    (%rsp),%rax
=> 0x000055b5bceba92c <+8716>:	mov    (%r8),%rsi
   0x000055b5bceba92f <+8719>:	xor    %edi,%edi
   0x000055b5bceba931 <+8721>:	mov    0x8(%rax),%rax
   0x000055b5bceba935 <+8725>:	mov    0x10(%rax),%rdx
   0x000055b5bceba939 <+8729>:	call   0x55b5bcf75d40 <install_branch_config>

(gdb) info registers r8 rsi
r8             0x0                 0
rsi            0x55b5bdbe430b      94239060804363

This is just a generic reproducer; original use case revolved around
the use of submodules (also happened to be in a detached state within,
"--set-upstream" attempt was then my rushed response to "You are
not currently on a branch." error message on a plain pull there).

> (Please cc me on replies, not subscribe to the mailing list).

(ditto, please)

-- poki

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

[Ævar Arnfjörð Bjarmason]

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 error 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".

Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Jul 19 2021, Jan Pokorný wrote:

> Hello,
>
> Clemens Fruhwirth píše v Po 05. 07. 2021 v 17:46 +0200:
>> What did you do before the bug happened? (Steps to reproduce your
>> issue)
>> 
>> Run "git pull origin nixos-unstable --rebase --set-upstream"
>> on a repo that had no branch set, e.g. when running "git branch" gave
>> "* (no branch)"
>> 

Thanks for the report both & sorry that this fell through the cracks
for so long.

The bug itself seems rather obvious given the benefit of that
reproduction scenario & backtrace, as noted above we've been playing
whack-a-mole with a related segfault since 2012.

It would be nice if you could test this patch and confirm, but given
the trivality of the segfault it shouldn't be necessary, I'm confident
that this fixes the bug you two reported. Thanks both!

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9191620e50c..c2eac8f15e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1623,6 +1623,17 @@ 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;
+				    
+				die(_("could not set upstream of HEAD to '%s' from '%s' when "
+				      "it does not point to any branch."),
+				    shortname, transport->remote->name);
+			}
+
 			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..ae90554645e 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 &&
+	fatal: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
+	EOF
+	test_must_fail git fetch --set-upstream upstream main 2>actual.raw &&
+	grep ^fatal: 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 &&
+	fatal: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
+	EOF
+	test_must_fail git pull --set-upstream upstream main 2>actual.raw &&
+	grep ^fatal: actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.32.0.874.ge7a9d58bfcf

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

[Junio C Hamano]

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

> 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 error 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".
>
> Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
> Reported-by: Jan Pokorný <poki@fnusa.cz>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Hmph, there is 5e0fa8cb (fetch: fix segfault on --set-upstream while
on a detached HEAD, 2021-07-06) from Clemens, which warns and skips
instead of dies.  Given that a handful of other conditions in this
function results in warnings "not setting upstream due to X", it
might be more in line with the spirit of the current system (it is
debatable if we should stress the fact that we did not set the
configuration with hard dying, though).


> ---
>
> On Mon, Jul 19 2021, Jan Pokorný wrote:
>
>> Hello,
>>
>> Clemens Fruhwirth píše v Po 05. 07. 2021 v 17:46 +0200:
>>> What did you do before the bug happened? (Steps to reproduce your
>>> issue)
>>> 
>>> Run "git pull origin nixos-unstable --rebase --set-upstream"
>>> on a repo that had no branch set, e.g. when running "git branch" gave
>>> "* (no branch)"
>>> 
>
> Thanks for the report both & sorry that this fell through the cracks
> for so long.
>
> The bug itself seems rather obvious given the benefit of that
> reproduction scenario & backtrace, as noted above we've been playing
> whack-a-mole with a related segfault since 2012.
>
> It would be nice if you could test this patch and confirm, but given
> the trivality of the segfault it shouldn't be necessary, I'm confident
> that this fixes the bug you two reported. Thanks both!
>
>  builtin/fetch.c         | 11 +++++++++++
>  t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 9191620e50c..c2eac8f15e5 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1623,6 +1623,17 @@ 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;
> +				    
> +				die(_("could not set upstream of HEAD to '%s' from '%s' when "
> +				      "it does not point to any branch."),
> +				    shortname, transport->remote->name);
> +			}
> +
>  			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..ae90554645e 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 &&
> +	fatal: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
> +	EOF
> +	test_must_fail git fetch --set-upstream upstream main 2>actual.raw &&
> +	grep ^fatal: 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 &&
> +	fatal: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
> +	EOF
> +	test_must_fail git pull --set-upstream upstream main 2>actual.raw &&
> +	grep ^fatal: actual.raw >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

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

[Ævar Arnfjörð Bjarmason]

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/20210706162238.575988-1-clemens@endorphin.org/
2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/

Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A proposed replacement of cf/fetch-set-upstream-while-detached which
as noted in What's Cooking has been languishing for a while. I changed
the die() in my version to a warning() as suggested by Junio & updated
the test and commit message accordingly.

Range-diff against v1:
1:  2d8f3e59e1f ! 1:  9e846b76959 pull, fetch: fix segfault in --set-upstream option
    @@ Commit message
         I'm fixing now in "git branch --set-upstream-to", see
         6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).
     
    -    The error message I'm adding here is an amalgamation of the error
    +    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/20210706162238.575988-1-clemens@endorphin.org/
    +    2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/
    +
         Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
         Reported-by: Jan Pokorný <poki@fnusa.cz>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
     +				if (!skip_prefix(source_ref->name,
     +						 "refs/heads/", &shortname))
     +					shortname = source_ref->name;
    -+				    
    -+				die(_("could not set upstream of HEAD to '%s' from '%s' when "
    -+				      "it does not point to any branch."),
    -+				    shortname, transport->remote->name);
    ++				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") ||
    @@ t/t5553-set-upstream.sh: test_expect_success 'fetch --set-upstream with valid UR
     +	git checkout HEAD^0 &&
     +	test_when_finished "git checkout -" &&
     +	cat >expect <<-\EOF &&
    -+	fatal: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
    ++	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
     +	EOF
    -+	test_must_fail git fetch --set-upstream upstream main 2>actual.raw &&
    -+	grep ^fatal: actual.raw >actual &&
    ++	git fetch --set-upstream upstream main 2>actual.raw &&
    ++	grep ^warning: actual.raw >actual &&
     +	test_cmp expect actual
     +'
     +
    @@ t/t5553-set-upstream.sh: test_expect_success 'pull --set-upstream with valid URL
     +	git checkout HEAD^0 &&
     +	test_when_finished "git checkout -" &&
     +	cat >expect <<-\EOF &&
    -+	fatal: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
    ++	warning: could not set upstream of HEAD to '"'"'main'"'"' from '"'"'upstream'"'"' when it does not point to any branch.
     +	EOF
    -+	test_must_fail git pull --set-upstream upstream main 2>actual.raw &&
    -+	grep ^fatal: actual.raw >actual &&
    ++	git pull --set-upstream upstream main 2>actual.raw &&
    ++	grep ^warning: actual.raw >actual &&
     +	test_cmp expect actual
     +'
     +

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 25740c13df1..ca487edd805 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1623,6 +1623,17 @@ 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;
+				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
-- 
2.33.0.662.gbc81f8cbdca

Re: [PATCH v2] pull, fetch: fix segfault in --set-upstream option

[Clemens Fruhwirth]

On Mon, 23 Aug 2021 at 14:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> A proposed replacement of cf/fetch-set-upstream-while-detached which
> as noted in What's Cooking has been languishing for a while. I changed
> the die() in my version to a warning() as suggested by Junio & updated
> the test and commit message accordingly.

Thank you Ævar, for taking care of that and adding tests and my
apologizes for not responding in a timely manner.

>  builtin/fetch.c         | 11 +++++++++++
>  t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 25740c13df1..ca487edd805 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1623,6 +1623,17 @@ 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;

Is skip_prefix ever able to fail?

- If yes, then shortname will be left pointing to NULL when we print
the warning below. Warning probably won't die because of NULL, but it
still will print something weird.
- If no, then an assert(!skip_prefix(source_ref->name, "refs/heads/",
&shortname)) might be more idiomatic.

> 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

Thank for you adding tests. LGTM.

Regards,
-- 
Fruhwirth Clemens http://clemens.endorphin.org

Re: [PATCH v2] pull, fetch: fix segfault in --set-upstream option

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 24 2021, Clemens Fruhwirth wrote:

> On Mon, 23 Aug 2021 at 14:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> A proposed replacement of cf/fetch-set-upstream-while-detached which
>> as noted in What's Cooking has been languishing for a while. I changed
>> the die() in my version to a warning() as suggested by Junio & updated
>> the test and commit message accordingly.
>
> Thank you Ævar, for taking care of that and adding tests and my
> apologizes for not responding in a timely manner.

No worries.

>>  builtin/fetch.c         | 11 +++++++++++
>>  t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 25740c13df1..ca487edd805 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1623,6 +1623,17 @@ 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;
>
> Is skip_prefix ever able to fail?
>
> - If yes, then shortname will be left pointing to NULL when we print
> the warning below. Warning probably won't die because of NULL, but it
> still will print something weird.

If it fails we'll use source_ref->name, so it won't be NULL...

> - If no, then an assert(!skip_prefix(source_ref->name, "refs/heads/",
> &shortname)) might be more idiomatic.

Which means that this would be a bug, since it's not handling the case
where the source doesn't start with refs/heads/*.

I.e. it's the same as doing it this way (on top of this v2), which
perhaps is a clearer way to express the same idea. What do you think?

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ca487edd805..2bc9159690d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1624,10 +1624,9 @@ 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);

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

[Ævar Arnfjörð Bjarmason]

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/20210706162238.575988-1-clemens@endorphin.org/
2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/

Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

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(+)

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
-- 
2.33.0.741.g4db85f1eb27

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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 v4] pull, fetch: fix segfault in --set-upstream option

[Ævar Arnfjörð Bjarmason]

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.

The --no-rebase option to "git pull" is needed as of the recently
merged 7d0daf3f12f (Merge branch 'en/pull-conflicting-options',
2021-08-30).

1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/
2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/

Reported-by: Clemens Fruhwirth <clemens@endorphin.org>
Reported-by: Jan Pokorný <poki@fnusa.cz>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A re-submission for a semantic conflict with the now-merged
7d0daf3f12f on master.

On Mon, Aug 30 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.

I see your point, but looking at it again I decided to keep it as-is
in this re-roll. As noted in the commit message this is for
consistency with the output we emit when running a commind like "git
branch --set-upstream-to <upstream> <branchname>", which as you'll see
if you "git checkout HEAD^0" we'll quote the wanted upstream back at
you.

So yeah, arguably we should just punt on that whole thing and tell the
user "you're on a detached HEAD, this will never work" or something
like that, but let's consider that UI change separately, and then do
it for all of "branch", "fetch" and "pull", rather than leave the
latter two inconsistent with "branch" with this fix.

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

Indeed. I changed it due to your upthread
<xmqqsg0anxix.fsf@gitster.g>.

I think doing s/warning/die/ here makes sense, but similarly to the
above comment: Let's punt on that and do it separately from this
narrow segfault fix. If and when we change that we should change
various other "warning()" around this codepath to "die()" as well.

Range-diff against v3:
1:  68899471206 ! 1:  0caa9a89a86 pull, fetch: fix segfault in --set-upstream option
    @@ Commit message
         detailed warning message here is better, and we should also have tests
         for this behavior.
     
    +    The --no-rebase option to "git pull" is needed as of the recently
    +    merged 7d0daf3f12f (Merge branch 'en/pull-conflicting-options',
    +    2021-08-30).
    +
         1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/
         2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/
     
    @@ t/t5553-set-upstream.sh: test_expect_success 'pull --set-upstream with valid URL
     +	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 &&
    ++	git pull --no-rebase --set-upstream upstream main 2>actual.raw &&
     +	grep ^warning: actual.raw >actual &&
     +	test_cmp expect actual
     +'

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

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 9c12c0f8c32..48050162c27 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 --no-rebase --set-upstream upstream main 2>actual.raw &&
+	grep ^warning: actual.raw >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.33.0.805.g739b16c2189

Re: [PATCH v4] pull, fetch: fix segfault in --set-upstream option

[Junio C Hamano]

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

>> 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.
>
> Indeed. I changed it due to your upthread
> <xmqqsg0anxix.fsf@gitster.g>.
>
> I think doing s/warning/die/ here makes sense, but similarly to the
> above comment: Let's punt on that and do it separately from this
> narrow segfault fix. If and when we change that we should change
> various other "warning()" around this codepath to "die()" as well.

I do not think that can be thrown into the same bin as "should UI
give irrelevant details?" issue.  If you make it not to segfault and
give just a warning(), it becomes impossible to make it die() later.

If we all agree that die() is a better action, that must be done
now, or it will become never once the change is released to the
field.

Re: [PATCH v4] pull, fetch: fix segfault in --set-upstream option

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 31 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>> 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.
>>
>> Indeed. I changed it due to your upthread
>> <xmqqsg0anxix.fsf@gitster.g>.
>>
>> I think doing s/warning/die/ here makes sense, but similarly to the
>> above comment: Let's punt on that and do it separately from this
>> narrow segfault fix. If and when we change that we should change
>> various other "warning()" around this codepath to "die()" as well.
>
> I do not think that can be thrown into the same bin as "should UI
> give irrelevant details?" issue.  If you make it not to segfault and
> give just a warning(), it becomes impossible to make it die() later.
>
> If we all agree that die() is a better action, that must be done
> now, or it will become never once the change is released to the
> field.

Because someone might have been relying on the current behavior of
segfaulting to stop their script? And a mere warning() would break
things for them by having the script "work" if this patch were to make
it into a release?

I think it's unlikely that anyone's running into this in the wild as
anything but a one-off, and in any case whether or not we segfault, warn
or die the behavior of fetch at this point is to have already finished
the fetch itself.

We're merely doing some post-fetch work of setting config etc. Both
before and after this patch we won't be setting the upstream config. But
yes, the exit code will change from a segfault to successful exit.

I think the first priority should be to just narrowly fix the segfault &
leave refactoring of e.g. having fetch do sanity checking on all options
before doing work for later, especially as we're almost 2 months into no
fix for the segfault landing on "master" after the first working patch
to fix it, so if we have that all wait on agreeing on the perfect
behavior for fetch error handling...

Re: [PATCH v4] pull, fetch: fix segfault in --set-upstream option

[Junio C Hamano]

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

>> If we all agree that die() is a better action, that must be done
>> now, or it will become never once the change is released to the
>> field.
>
> Because someone might have been relying on the current behavior of
> segfaulting to stop their script? And a mere warning() would break
> things for them by having the script "work" if this patch were to make
> it into a release?

No, because someone WILL start rely on the warning() behaviour,
expecting that the "fixed" command will now run to completion
without exiting with non-zero status.  Once that happens, it will
become impossible to flip it to die().