{"thread":{"id":"54237","subject":"bug found on the new git maintenance builtin command","startedAt":"2020-09-14T19:49:47Z","lastAt":"2020-09-14T20:55:08Z","messageCount":3,"participants":["Rafael Silva","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"405520","messageId":"20200914194938.GA4235@contrib-buster.localdomain","threadId":"54237","inReplyTo":null,"subject":"bug found on the new git maintenance builtin command","fromName":"Rafael Silva","fromEmail":"rafaeloliveira.cs@gmail.com","sentAt":"2020-09-14T19:49:38Z","receivedAt":"2020-09-14T19:49:47Z","isPatch":false,"sender":{"key":"rafaeloliveira.cs@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5935135?v=4"},"body":"Hi Everyone,\n\nI found a minor bug when testing the new maintenance built-in command that was\nintroduced on 679768e2a1 (maintenance: create basic maintenance runner, 2020-08-25) submitted in [1]\n\nWhen running `git maintenance` without argument it receives\na segmentation fault. I'm running git built on the current `seen` branch that is pointing\nto bf3e2864f3 (Merge branch 'ds/maintenance-part-3' into seen, 2020-09-11)\n\nI did a little investigation and it seems the command needs to check when no arguments\nare passed to the cmd_maintanance() function and either quit with usage or redirect to man.\n\nTo reproduce the error, just run the `git maintenance` without any arguments\n\n$ git maintenance\nSegmentation fault\n\nAnalysign with GDB right before the SEGFAULT are thrown, we can see\nthe argv[1] pointing to NULL as follows: \n\n(gdb) list\n1628    int cmd_maintenance(int argc, const char **argv, const char *prefix)\n1629    {\n1630        if (argc == 2 && !strcmp(argv[1], \"-h\"))\n1631            usage(builtin_maintenance_usage);\n1632\n1633        fprintf(stdout, \"run\");\n1634        if (!strcmp(argv[1], \"run\"))\n1635            return maintenance_run(argc - 1, argv + 1, prefix);\n1636        if (!strcmp(argv[1], \"start\"))\n1637            return maintenance_start();\n(gdb) print argc\n$5 = 1\n(gdb) print argv[1]\n$6 = 0x0\n\nHope all this information helps with the fixing it\n\n[1] Patch submission can be found in:\nhttps://public-inbox.org/git/aa961af387b7f458f75ad60b9a2a45da4bb43794.1599224956.git.gitgitgadget@gmail.com/\n\nRegards,\nRafael Silva\n"},{"id":"405533","messageId":"0b35829f-a83b-a093-2dc5-0e7d3b42fd15@gmail.com","threadId":"54237","inReplyTo":"20200914194938.GA4235@contrib-buster.localdomain","subject":"Re: bug found on the new git maintenance builtin command","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-14T20:48:35Z","receivedAt":"2020-09-14T20:48:41Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/14/2020 3:49 PM, Rafael Silva wrote:\n> Hi Everyone,\n> \n> I found a minor bug when testing the new maintenance built-in command that was\n> introduced on 679768e2a1 (maintenance: create basic maintenance runner, 2020-08-25) submitted in [1]\n\nThank you for identifying the original patch! My gut reaction was that\nthis is just in the Part III code which adds subcommands, but that is\nincorrect.\n\n> (gdb) list\n> 1628    int cmd_maintenance(int argc, const char **argv, const char *prefix)\n> 1629    {\n> 1630        if (argc == 2 && !strcmp(argv[1], \"-h\"))\n> 1631            usage(builtin_maintenance_usage);\n> 1632\n> 1633        fprintf(stdout, \"run\");\n> 1634        if (!strcmp(argv[1], \"run\"))\n> 1635            return maintenance_run(argc - 1, argv + 1, prefix);\n> 1636        if (!strcmp(argv[1], \"start\"))\n> 1637            return maintenance_start();\n> (gdb) print argc\n> $5 = 1\n> (gdb) print argv[1]\n> $6 = 0x0\n> \n> Hope all this information helps with the fixing it\n\nThank you so much for the report!\n\nThe patch below applies to ds/maintenance-part-1, to fix the problem.\nHopefully it also merges cleanly with the changes in ds/maintenance-part-3,\nbut I can deal with that when I submit my next re-roll.\n\nThanks!\n-Stolee\n\n-- >8 --\n\nFrom 8cd793e16cd8521f4f8d7ccf2b93492ba444e8e7 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee <dstolee@microsoft.com>\nDate: Mon, 14 Sep 2020 16:42:36 -0400\nSubject: [PATCH] maintenance: correctly handle missing subcommand\n\nThe maintenance builtin created in 679768e2a12 (maintenance: create\nbasic maintenance runner, 2020-08-25) has a flaw in that it does not\nprotect against a user running \"git maintenance\" without any additional\nparameters. Correct this by adding a check on argc before looking for\nthe -h option.\n\nReported-by: Rafael Silva <rafaeloliveira.cs@gmail.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/gc.c           | 3 ++-\n t/t7900-maintenance.sh | 4 +++-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c3bcdc1167a..090959350e0 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1027,7 +1027,8 @@ static const char builtin_maintenance_usage[] = N_(\"git maintenance run [<option\n \n int cmd_maintenance(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (argc < 2 ||\n+\t    (argc == 2 && !strcmp(argv[1], \"-h\")))\n \t\tusage(builtin_maintenance_usage);\n \n \tif (!strcmp(argv[1], \"run\"))\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 4f6a04ddb1e..53c883531e4 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -10,7 +10,9 @@ test_expect_success 'help text' '\n \ttest_expect_code 129 git maintenance -h 2>err &&\n \ttest_i18ngrep \"usage: git maintenance run\" err &&\n \ttest_expect_code 128 git maintenance barf 2>err &&\n-\ttest_i18ngrep \"invalid subcommand: barf\" err\n+\ttest_i18ngrep \"invalid subcommand: barf\" err &&\n+\ttest_expect_code 129 git maintenance 2>err &&\n+\ttest_i18ngrep \"usage: git maintenance\" err\n '\n \n test_expect_success 'run [--auto|--quiet]' '\n-- \n2.28.0.vfs.0.0\n\n\n\n"},{"id":"405534","messageId":"xmqq7dsw2i57.fsf@gitster.c.googlers.com","threadId":"54237","inReplyTo":"0b35829f-a83b-a093-2dc5-0e7d3b42fd15@gmail.com","subject":"Re: bug found on the new git maintenance builtin command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T20:55:00Z","receivedAt":"2020-09-14T20:55:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 9/14/2020 3:49 PM, Rafael Silva wrote:\n>> Hi Everyone,\n>> \n>> I found a minor bug when testing the new maintenance built-in command that was\n>> introduced on 679768e2a1 (maintenance: create basic maintenance runner, 2020-08-25) submitted in [1]\n>\n> Thank you for identifying the original patch! My gut reaction was that\n> this is just in the Part III code which adds subcommands, but that is\n> incorrect.\n>\n>> (gdb) list\n>> 1628    int cmd_maintenance(int argc, const char **argv, const char *prefix)\n>> 1629    {\n>> 1630        if (argc == 2 && !strcmp(argv[1], \"-h\"))\n>> 1631            usage(builtin_maintenance_usage);\n>> 1632\n>> 1633        fprintf(stdout, \"run\");\n>> 1634        if (!strcmp(argv[1], \"run\"))\n>> 1635            return maintenance_run(argc - 1, argv + 1, prefix);\n>> 1636        if (!strcmp(argv[1], \"start\"))\n>> 1637            return maintenance_start();\n>> (gdb) print argc\n>> $5 = 1\n>> (gdb) print argv[1]\n>> $6 = 0x0\n>> \n>> Hope all this information helps with the fixing it\n>\n> Thank you so much for the report!\n>\n> The patch below applies to ds/maintenance-part-1, to fix the problem.\n> Hopefully it also merges cleanly with the changes in ds/maintenance-part-3,\n> but I can deal with that when I submit my next re-roll.\n\nYuck.  I am pretty sure that I did spot this myself during the\nreview cycle, but apparently it slipped through X-<.\n\nWill apply.  Thanks.\n"}]}