bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

[Gregory David]

[-- Attachment #1.1.1: Type: text/plain, Size: 1424 bytes --]

Hi, this bugreport is about a SEGFAULT on git binary when running in
this repository (for example, as it also segfault on other private
repositories): https://framagit.org/groolot-association/osc_looper.git


What did you do before the bug happened? (Steps to reproduce your
issue) I just wanted to list branches, and so playing with 'git
show-branch --current --reflog=3' in the repository
https://framagit.org/groolot-association/osc_looper.git

What did you expect to happen? (Expected behavior)
Not to segfault

What happened instead? (Actual behavior)
This command fails with a SEGFAULT on version 2.35.1, 2.20.1 and 2.17.1
It also SEGFAULT on branch 'next' of github git repository.

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

Anything else you want to add:
Segfaut happened when '--current' AND '--reflog=3' are provided together

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.35.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.16.0-4-amd64 #1 SMP PREEMPT Debian 5.16.12-1 (2022-03-08)
x86_64
compiler info: gnuc: 11.2
libc info: glibc: 2.33
$SHELL (typically, interactive shell): /usr/bin/zsh


Best regards

-- 
Gregory David
Security Engineer
https://www.p1sec.com

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2501 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 13 2022, Gregory David wrote:

> [[PGP Signed Part:Undecided]]
> Hi, this bugreport is about a SEGFAULT on git binary when running in
> this repository (for example, as it also segfault on other private
> repositories): https://framagit.org/groolot-association/osc_looper.git
>
>
> What did you do before the bug happened? (Steps to reproduce your
> issue) I just wanted to list branches, and so playing with 'git
> show-branch --current --reflog=3' in the repository
> https://framagit.org/groolot-association/osc_looper.git
>
> What did you expect to happen? (Expected behavior)
> Not to segfault
>
> What happened instead? (Actual behavior)
> This command fails with a SEGFAULT on version 2.35.1, 2.20.1 and 2.17.1
> It also SEGFAULT on branch 'next' of github git repository.
>
> What's different between what you expected and what actually happened?
> Segfault
>
> Anything else you want to add:
> Segfaut happened when '--current' AND '--reflog=3' are provided together
>
> 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.35.1
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> uname: Linux 5.16.0-4-amd64 #1 SMP PREEMPT Debian 5.16.12-1 (2022-03-08)
> x86_64
> compiler info: gnuc: 11.2
> libc info: glibc: 2.33
> $SHELL (typically, interactive shell): /usr/bin/zsh

This appears to fix it, but I didn't have time to further validate it,
come up with a test etc:

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 330b0553b9d..be3890b2dd6 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -881,10 +881,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				       get_color_reset_code(), ref_name[i]);
 			}
 
-			if (!reflog) {
+			if (!reflog || !reflog_msg[i])
 				/* header lines never need name */
 				show_one_commit(rev[i], 1);
-			}
 			else
 				puts(reflog_msg[i]);
 

Re: bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

[Gregory David]

[-- Attachment #1.1.1: Type: text/plain, Size: 2951 bytes --]

Thanks a lot Aevar!

Don't you mean this:

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 330b0553b9..2a5c31729d 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -881,7 +881,7 @@ int cmd_show_branch(int ac, const char **av, const
char *prefix)
                                       get_color_reset_code(), ref_name[i]);
                        }

-                       if (!reflog) {
+                       if (!reflog || !rev[i]) {
                                /* header lines never need name */
                                show_one_commit(rev[i], 1);
                        }

Best regards.

On 13/04/2022 13:57, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 13 2022, Gregory David wrote:
> 
>> [[PGP Signed Part:Undecided]]
>> Hi, this bugreport is about a SEGFAULT on git binary when running in
>> this repository (for example, as it also segfault on other private
>> repositories): https://framagit.org/groolot-association/osc_looper.git
>>
>>
>> What did you do before the bug happened? (Steps to reproduce your
>> issue) I just wanted to list branches, and so playing with 'git
>> show-branch --current --reflog=3' in the repository
>> https://framagit.org/groolot-association/osc_looper.git
>>
>> What did you expect to happen? (Expected behavior)
>> Not to segfault
>>
>> What happened instead? (Actual behavior)
>> This command fails with a SEGFAULT on version 2.35.1, 2.20.1 and 2.17.1
>> It also SEGFAULT on branch 'next' of github git repository.
>>
>> What's different between what you expected and what actually happened?
>> Segfault
>>
>> Anything else you want to add:
>> Segfaut happened when '--current' AND '--reflog=3' are provided together
>>
>> 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.35.1
>> cpu: x86_64
>> no commit associated with this build
>> sizeof-long: 8
>> sizeof-size_t: 8
>> shell-path: /bin/sh
>> uname: Linux 5.16.0-4-amd64 #1 SMP PREEMPT Debian 5.16.12-1 (2022-03-08)
>> x86_64
>> compiler info: gnuc: 11.2
>> libc info: glibc: 2.33
>> $SHELL (typically, interactive shell): /usr/bin/zsh
> 
> This appears to fix it, but I didn't have time to further validate it,
> come up with a test etc:
> 
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 330b0553b9d..be3890b2dd6 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -881,10 +881,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  				       get_color_reset_code(), ref_name[i]);
>  			}
>  
> -			if (!reflog) {
> +			if (!reflog || !reflog_msg[i])
>  				/* header lines never need name */
>  				show_one_commit(rev[i], 1);
> -			}
>  			else
>  				puts(reflog_msg[i]);
>  
> 

-- 
Gregory David
Security Engineer
https://www.p1sec.com

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2501 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 13 2022, Gregory David wrote:

> [[PGP Signed Part:Undecided]]
> Thanks a lot Aevar!
>
> Don't you mean this:
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 330b0553b9..2a5c31729d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -881,7 +881,7 @@ int cmd_show_branch(int ac, const char **av, const
> char *prefix)
>                                        get_color_reset_code(), ref_name[i]);
>                         }
>
> -                       if (!reflog) {
> +                       if (!reflog || !rev[i]) {
>                                 /* header lines never need name */
>                                 show_one_commit(rev[i], 1);

No, but my "suggested" fix was just enough to compile and get past the
segfault, i.e. we don't reach the puts(reflog_msg[i]) branch, but of
course rev[i] may be NULL too, I didn't check.

For this one though: I haven't tried it, but reading show_one_commit()
one of the first things we do is dereference the 1st argument, so surely
that will segfault too...

Re: bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

[Gregory David]

[-- Attachment #1.1.1: Type: text/plain, Size: 1519 bytes --]

By my side, your first suggestion didn't remove the segfault due to the
irrelevant check on 'reflog[i]' that is not use on next line, so unable
to segfault. The fact is you pointed the exact place where it happens
and my fix of your fix :) remove the segfault.

On 13/04/2022 18:46, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 13 2022, Gregory David wrote:
> 
>> [[PGP Signed Part:Undecided]]
>> Thanks a lot Aevar!
>>
>> Don't you mean this:
>>
>> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
>> index 330b0553b9..2a5c31729d 100644
>> --- a/builtin/show-branch.c
>> +++ b/builtin/show-branch.c
>> @@ -881,7 +881,7 @@ int cmd_show_branch(int ac, const char **av, const
>> char *prefix)
>>                                        get_color_reset_code(), ref_name[i]);
>>                         }
>>
>> -                       if (!reflog) {
>> +                       if (!reflog || !rev[i]) {
>>                                 /* header lines never need name */
>>                                 show_one_commit(rev[i], 1);
> 
> No, but my "suggested" fix was just enough to compile and get past the
> segfault, i.e. we don't reach the puts(reflog_msg[i]) branch, but of
> course rev[i] may be NULL too, I didn't check.
> 
> For this one though: I haven't tried it, but reading show_one_commit()
> one of the first things we do is dereference the 1st argument, so surely
> that will segfault too...

-- 
Gregory David
Security Engineer
https://www.p1sec.com

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2501 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: bugreport - SEGFAULT on 'git show-branch --current --reflog=3'

[Gregory David]

[-- Attachment #1.1.1: Type: text/plain, Size: 1297 bytes --]

Sorry, my last answer is totally garbage.

On 13/04/2022 18:46, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 13 2022, Gregory David wrote:
> 
>> [[PGP Signed Part:Undecided]]
>> Thanks a lot Aevar!
>>
>> Don't you mean this:
>>
>> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
>> index 330b0553b9..2a5c31729d 100644
>> --- a/builtin/show-branch.c
>> +++ b/builtin/show-branch.c
>> @@ -881,7 +881,7 @@ int cmd_show_branch(int ac, const char **av, const
>> char *prefix)
>>                                        get_color_reset_code(), ref_name[i]);
>>                         }
>>
>> -                       if (!reflog) {
>> +                       if (!reflog || !rev[i]) {
>>                                 /* header lines never need name */
>>                                 show_one_commit(rev[i], 1);
> 
> No, but my "suggested" fix was just enough to compile and get past the
> segfault, i.e. we don't reach the puts(reflog_msg[i]) branch, but of
> course rev[i] may be NULL too, I didn't check.
> 
> For this one though: I haven't tried it, but reading show_one_commit()
> one of the first things we do is dereference the 1st argument, so surely
> that will segfault too...

-- 
Gregory David
Security Engineer
https://www.p1sec.com

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2501 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]