threads / bug / 54237

bug found on the new git maintenance builtin command

Subject: bug found on the new git maintenance builtin command

## tl;dr

3 messages between Sep 14, 2020 and Sep 14, 2020.

replies: 2people: 3as markdown or json

Rafael Silva· Sep 14, 2020, 19:49 UTC · lore

Hi Everyone,

I found a minor bug when testing the new maintenance built-in command that was introduced on 679768e2a1 (maintenance: create basic maintenance runner, 2020-08-25) submitted in [1]

When running `git maintenance` without argument it receives a segmentation fault. I'm running git built on the current `seen` branch that is pointing to bf3e2864f3 (Merge branch 'ds/maintenance-part-3' into seen, 2020-09-11)

I did a little investigation and it seems the command needs to check when no arguments are passed to the cmd_maintanance() function and either quit with usage or redirect to man.

To reproduce the error, just run the `git maintenance` without any arguments

$ git maintenance Segmentation fault

Analysign with GDB right before the SEGFAULT are thrown, we can see the argv[1] pointing to NULL as follows:

(gdb) list 1628 int cmd_maintenance(int argc, const char **argv, const char *prefix) 1629 { 1630 if (argc == 2 && !strcmp(argv[1], "-h")) 1631 usage(builtin_maintenance_usage); 1632 1633 fprintf(stdout, "run"); 1634 if (!strcmp(argv[1], "run")) 1635 return maintenance_run(argc - 1, argv + 1, prefix); 1636 if (!strcmp(argv[1], "start")) 1637 return maintenance_start(); (gdb) print argc $5 = 1 (gdb) print argv[1] $6 = 0x0

Hope all this information helps with the fixing it

[1] Patch submission can be found in: https://public-inbox.org/git/aa961af387b7f458f75ad60b9a2a45da4bb43794.1599224956.git.gitgitgadget@gmail.com/

Regards, Rafael Silva

Derrick Stolee· Sep 14, 2020, 20:48 UTC · re: Rafael Silva · lore

Re: bug found on the new git maintenance builtin command

On 9/14/2020 3:49 PM, Rafael Silva wrote:

> Hi Everyone,
> 
> I found a minor bug when testing the new maintenance built-in command that was
> introduced on 679768e2a1 (maintenance: create basic maintenance runner, 2020-08-25) submitted in [1]

Thank you for identifying the original patch! My gut reaction was that this is just in the Part III code which adds subcommands, but that is incorrect.

Show 17 quoted lines

> (gdb) list
> 1628    int cmd_maintenance(int argc, const char **argv, const char *prefix)
> 1629    {
> 1630        if (argc == 2 && !strcmp(argv[1], "-h"))
> 1631            usage(builtin_maintenance_usage);
> 1632
> 1633        fprintf(stdout, "run");
> 1634        if (!strcmp(argv[1], "run"))
> 1635            return maintenance_run(argc - 1, argv + 1, prefix);
> 1636        if (!strcmp(argv[1], "start"))
> 1637            return maintenance_start();
> (gdb) print argc
> $5 = 1
> (gdb) print argv[1]
> $6 = 0x0
> 
> Hope all this information helps with the fixing it

Thank you so much for the report!

The patch below applies to ds/maintenance-part-1, to fix the problem. Hopefully it also merges cleanly with the changes in ds/maintenance-part-3, but I can deal with that when I submit my next re-roll.

Thanks! -Stolee

-- >8 --

From 8cd793e16cd8521f4f8d7ccf2b93492ba444e8e7 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Mon, 14 Sep 2020 16:42:36 -0400
Subject: [PATCH] maintenance: correctly handle missing subcommand

The maintenance builtin created in 679768e2a12 (maintenance: create basic maintenance runner, 2020-08-25) has a flaw in that it does not protect against a user running "git maintenance" without any additional parameters. Correct this by adding a check on argc before looking for the -h option.

Reported-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/gc.c           | 3 ++-
 t/t7900-maintenance.sh | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c3bcdc1167a..090959350e0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1027,7 +1027,8 @@ static const char builtin_maintenance_usage[] = N_("git maintenance run [<option
 
 int cmd_maintenance(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (argc < 2 ||
+	    (argc == 2 && !strcmp(argv[1], "-h")))
 		usage(builtin_maintenance_usage);
 
 	if (!strcmp(argv[1], "run"))
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4f6a04ddb1e..53c883531e4 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -10,7 +10,9 @@ test_expect_success 'help text' '
 	test_expect_code 129 git maintenance -h 2>err &&
 	test_i18ngrep "usage: git maintenance run" err &&
 	test_expect_code 128 git maintenance barf 2>err &&
-	test_i18ngrep "invalid subcommand: barf" err
+	test_i18ngrep "invalid subcommand: barf" err &&
+	test_expect_code 129 git maintenance 2>err &&
+	test_i18ngrep "usage: git maintenance" err
 '
 
 test_expect_success 'run [--auto|--quiet]' '

-- 
2.28.0.vfs.0.0

Junio C Hamano· Sep 14, 2020, 20:55 UTC · re: Derrick Stolee · lore

Re: bug found on the new git maintenance builtin command

Derrick Stolee <stolee@gmail.com> writes:

Show 33 quoted lines

> On 9/14/2020 3:49 PM, Rafael Silva wrote:
>> Hi Everyone,
>> 
>> I found a minor bug when testing the new maintenance built-in command that was
>> introduced on 679768e2a1 (maintenance: create basic maintenance runner, 2020-08-25) submitted in [1]
>
> Thank you for identifying the original patch! My gut reaction was that
> this is just in the Part III code which adds subcommands, but that is
> incorrect.
>
>> (gdb) list
>> 1628    int cmd_maintenance(int argc, const char **argv, const char *prefix)
>> 1629    {
>> 1630        if (argc == 2 && !strcmp(argv[1], "-h"))
>> 1631            usage(builtin_maintenance_usage);
>> 1632
>> 1633        fprintf(stdout, "run");
>> 1634        if (!strcmp(argv[1], "run"))
>> 1635            return maintenance_run(argc - 1, argv + 1, prefix);
>> 1636        if (!strcmp(argv[1], "start"))
>> 1637            return maintenance_start();
>> (gdb) print argc
>> $5 = 1
>> (gdb) print argv[1]
>> $6 = 0x0
>> 
>> Hope all this information helps with the fixing it
>
> Thank you so much for the report!
>
> The patch below applies to ds/maintenance-part-1, to fix the problem.
> Hopefully it also merges cleanly with the changes in ds/maintenance-part-3,
> but I can deal with that when I submit my next re-roll.

Yuck. I am pretty sure that I did spot this myself during the review cycle, but apparently it slipped through X-<.

Will apply.  Thanks.