{"thread":{"id":"62960","subject":"Crash on empty pickaxe argument","startedAt":"2025-02-17T01:24:17Z","lastAt":"2025-02-18T19:29:06Z","messageCount":6,"participants":["Jared Van Bortel","brian m. carlson","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"512494","messageId":"b59b75a1ad77f59cc8d81e2dd5cc9607e53b50af.camel@gmail.com","threadId":"62960","inReplyTo":null,"subject":"Crash on empty pickaxe argument","fromName":"Jared Van Bortel","fromEmail":"cebtenzzre@gmail.com","sentAt":"2025-02-17T01:24:14Z","receivedAt":"2025-02-17T01:24:17Z","isPatch":false,"sender":{"key":"cebtenzzre@gmail.com","avatar":null},"body":"Hi all,\n\nI fat-fingered my enter key today and noticed this crash:\n\n$ git --version\ngit version 2.48.1\n$ mkdir foo && cd foo\n$ git init\n\n$ git commit --allow-empty -m \"foo\"\n[master (root-commit) ed4e080] foo\n$ git log -S \"\"\nBUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n[1] 65421 IOT instruction (core dumped) git log -S \"\"\n$ git log -G \"\"\nBUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n[1] 65457 IOT instruction (core dumped) git log -G \"\"\n\n\nWell, it says BUG right there. It probably shouldn't crash. Relevant\nbacktrace for -S:\n\n#3 0x00007ffff7ce7582 in __GI_abort () at abort.c:73\n#4 0x00005555558689aa in BUG_vfl (file=, line=, fmt=0x5555558ed498 \"should have needle under -G or -S\", params=0x7fffffffbf70)\n at /usr/src/debug/git/git-2.48.1/usage.c:318\n#5 BUG_fl (file=, line=, fmt=0x5555558ed498 \"should have needle under -G or -S\") at /usr/src/debug/git/git-2.48.1/usage.c:335\n#6 0x000055555571527f in diffcore_pickaxe (o=0x7fffffffca90) at /usr/src/debug/git/git-2.48.1/diffcore-pickaxe.c:241\n#7 diffcore_std (options=0x7fffffffca90) at /usr/src/debug/git/git-2.48.1/diff.c:7079\n#8 0x00005555558b2788 in log_tree_diff_flush.isra.0 (opt=0x7fffffffc4d0) at /usr/src/debug/git/git-2.48.1/log-tree.c:929\n#9 0x000055555574b2e4 in log_tree_diff (opt=0x7fffffffc4d0, commit=0x5555559b7b80, log=0x7fffffffc300) at /usr/src/debug/git/git-2.48.1/log-tree.c:1120\n#10 log_tree_commit (opt=0x7fffffffc4d0, commit=0x5555559b7b80) at /usr/src/debug/git/git-2.48.1/log-tree.c:1188\n#11 0x00005555555e68c8 in cmd_log_walk_no_free (rev=) at builtin/log.c:528\n#12 cmd_log_walk_no_free (rev=rev@entry=0x7fffffffc4d0) at builtin/log.c:506\n#13 0x00005555555e74bf in cmd_log_walk (rev=0x7fffffffc4d0) at builtin/log.c:569\n#14 cmd_log (argc=3, argv=0x5555559b0790, prefix=0x0, repo=) at builtin/log.c:950\n#15 0x000055555555e56c in run_builtin (p=0x555555977f40 , argc=, argv=, repo=0x555555986d40 )\n at /usr/src/debug/git/git-2.48.1/git.c:480\n#16 handle_builtin (args=args@entry=0x7fffffffd4a0) at /usr/src/debug/git/git-2.48.1/git.c:740\n#17 0x000055555555ec34 in run_argv (args=0x7fffffffd4a0) at /usr/src/debug/git/git-2.48.1/git.c:807\n#18 0x00005555555598a1 in cmd_main (argc=, argv=) at /usr/src/debug/git/git-2.48.1/git.c:947\n#19 main (argc=, argv=) at /usr/src/debug/git/git-2.48.1/common-main.c:64\n\n\nThanks,\nJared\n"},{"id":"512495","messageId":"Z7KX2y-dXaZuPrW4@tapette.crustytoothpaste.net","threadId":"62960","inReplyTo":"b59b75a1ad77f59cc8d81e2dd5cc9607e53b50af.camel@gmail.com","subject":"Re: Crash on empty pickaxe argument","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-02-17T01:58:51Z","receivedAt":"2025-02-17T01:59:00Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-02-17 at 01:24:14, Jared Van Bortel wrote:\n> Hi all,\n\nHi,\n\n> I fat-fingered my enter key today and noticed this crash:\n> \n> $ git --version\n> git version 2.48.1\n> $ mkdir foo && cd foo\n> $ git init\n> \n> $ git commit --allow-empty -m \"foo\"\n> [master (root-commit) ed4e080] foo\n> $ git log -S \"\"\n> BUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n> [1] 65421 IOT instruction (core dumped) git log -S \"\"\n> $ git log -G \"\"\n> BUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n> [1] 65457 IOT instruction (core dumped) git log -G \"\"\n> \n> \n> Well, it says BUG right there. It probably shouldn't crash. Relevant\n> backtrace for -S:\n\nThanks for the report. I agree this is a bug and we shouldn't crash. I\nthink the right thing to do here is reject empty arguments since those\naren't actually very useful, so I'm in the process of writing a patch to\ndo that now, which I'll send out either later this evening or tomorrow,\ndepending on when CI finishes running.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"512542","messageId":"20250217175759.1576684-1-sandals@crustytoothpaste.net","threadId":"62960","inReplyTo":"Z7KX2y-dXaZuPrW4@tapette.crustytoothpaste.net","subject":"[PATCH] diff: don't crash with empty argument to -G or -S","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-02-17T17:57:59Z","receivedAt":"2025-02-17T17:58:10Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The pickaxe options, -G and -S, need either a regex or a string to look\nthrough the history for. An empty value isn't very useful since it\nwould either match everything or nothing, and what's worse, we presently\ncrash with a BUG like so when the user provides one:\n\n BUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n\nSince it's not very nice of us to crash and this wouldn't do anything\nuseful anyway, let's simply inform the user that they must provide a\nnon-empty argument and exit with an error if they provide an empty one\ninstead.\n\nReported-by: Jared Van Bortel \nSigned-off-by: brian m. carlson \n---\n diff.c | 4 ++++\n t/t4209-log-pickaxe.sh | 16 ++++++++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 019fb893a7..c89c15d98e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt,\n \tBUG_ON_OPT_NEG(unset);\n \toptions->pickaxe = arg;\n \toptions->pickaxe_opts |= DIFF_PICKAXE_KIND_G;\n+\tif (arg && !*arg)\n+\t\treturn error(_(\"-G requires a non-empty argument\"));\n \treturn 0;\n }\n \n@@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt,\n \tBUG_ON_OPT_NEG(unset);\n \toptions->pickaxe = arg;\n \toptions->pickaxe_opts |= DIFF_PICKAXE_KIND_S;\n+\tif (arg && !*arg)\n+\t\treturn error(_(\"-S requires a non-empty argument\"));\n \treturn 0;\n }\n \ndiff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\nindex a675ace081..0e2f80a268 100755\n--- a/t/t4209-log-pickaxe.sh\n+++ b/t/t4209-log-pickaxe.sh\n@@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'usage: -G and -S with empty argument' '\n+\tcat >expect <<-\\EOF &&\n+\terror: -S requires a non-empty argument\n+\tEOF\n+\n+\ttest_expect_code 129 git log -S \"\" 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\terror: -G requires a non-empty argument\n+\tEOF\n+\n+\ttest_expect_code 129 git log -G \"\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_log\texpect_initial\t--grep initial\n test_log\texpect_nomatch\t--grep InItial\n test_log_icase\texpect_initial\t--grep InItial\n"},{"id":"512547","messageId":"CABPp-BF01SYAr8TNxMBNvxPKseSqLH4mVC=MrrVp8w9ejEzK1g@mail.gmail.com","threadId":"62960","inReplyTo":"20250217175759.1576684-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: don't crash with empty argument to -G or -S","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-17T22:18:12Z","receivedAt":"2025-02-17T22:18:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 17, 2025 at 9:58 AM brian m. carlson\n wrote:\n>\n> The pickaxe options, -G and -S, need either a regex or a string to look\n> through the history for. An empty value isn't very useful since it\n> would either match everything or nothing, and what's worse, we presently\n> crash with a BUG like so when the user provides one:\n>\n> BUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n>\n> Since it's not very nice of us to crash and this wouldn't do anything\n> useful anyway, let's simply inform the user that they must provide a\n> non-empty argument and exit with an error if they provide an empty one\n> instead.\n\nMakes sense.\n\n> Reported-by: Jared Van Bortel \n> Signed-off-by: brian m. carlson \n> ---\n> diff.c | 4 ++++\n> t/t4209-log-pickaxe.sh | 16 ++++++++++++++++\n> 2 files changed, 20 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 019fb893a7..c89c15d98e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt,\n> BUG_ON_OPT_NEG(unset);\n> options->pickaxe = arg;\n> options->pickaxe_opts |= DIFF_PICKAXE_KIND_G;\n> + if (arg && !*arg)\n> + return error(_(\"-G requires a non-empty argument\"));\n> return 0;\n> }\n>\n> @@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt,\n> BUG_ON_OPT_NEG(unset);\n> options->pickaxe = arg;\n> options->pickaxe_opts |= DIFF_PICKAXE_KIND_S;\n> + if (arg && !*arg)\n> + return error(_(\"-S requires a non-empty argument\"));\n> return 0;\n> }\n\nSimple fix, as expected.\n\n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index a675ace081..0e2f80a268 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'usage: -G and -S with empty argument' '\n> + cat >expect <<-\\EOF &&\n> + error: -S requires a non-empty argument\n> + EOF\n> +\n> + test_expect_code 129 git log -S \"\" 2>actual &&\n> + test_cmp expect actual &&\n> +\n> + cat >expect <<-\\EOF &&\n> + error: -G requires a non-empty argument\n> + EOF\n> +\n> + test_expect_code 129 git log -G \"\" 2>actual &&\n> + test_cmp expect actual\n> +'\n\nLooks good to me.\n"},{"id":"512629","messageId":"xmqqbjuzxgn3.fsf@gitster.g","threadId":"62960","inReplyTo":"20250217175759.1576684-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] diff: don't crash with empty argument to -G or -S","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-18T18:16:32Z","receivedAt":"2025-02-18T18:16:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> The pickaxe options, -G and -S, need either a regex or a string to look\n> through the history for. An empty value isn't very useful since it\n> would either match everything or nothing, and what's worse, we presently\n> crash with a BUG like so when the user provides one:\n>\n> BUG: diffcore-pickaxe.c:241: should have needle under -G or -S\n\nI agree BUG is unwelcome. I am not sure about the value of\nforbidding an empty string (I am sure about forbidding NULL,\nthough). \n\nIf an empty matches everything, \"git log -S\" would skip changes that\nwould keep the number of lines, right? For the history of a project\nthat keeps track of source code, such a \"feature\" would not be\nuseful, but I can see a complaint by somebody who may want to keep\ntrack of a \"list of things\" one-item-per-line, if we had been\nallowing an empty string. It would be a regression for such a niche\nuser.\n\nLuckily, since we have stopped with a \"BUG\", we do not have to worry\nabout backward compatibility in this case ;-)\n\n> Since it's not very nice of us to crash and this wouldn't do anything\n> useful anyway, let's simply inform the user that they must provide a\n> non-empty argument and exit with an error if they provide an empty one\n> instead.\n\nSo I'd say that it may be a bit premature for us to declare\n\"anything useful\", I am perfectly fine with the patch given here.\nIf somebody who wants to maintain a text file, one-item-per-line\nthat keeps track of a list of things to omit commits that do not\nchange the number of items, they can drop \"&& !*arg\" part, tweak the\nmessage and add their own tests, once this fix lands and the dust\nsettles.\n\nThanks for a quick fix. Will queue.\n\n>\n> Reported-by: Jared Van Bortel \n> Signed-off-by: brian m. carlson \n> ---\n> diff.c | 4 ++++\n> t/t4209-log-pickaxe.sh | 16 ++++++++++++++++\n> 2 files changed, 20 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index 019fb893a7..c89c15d98e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt,\n> \tBUG_ON_OPT_NEG(unset);\n> \toptions->pickaxe = arg;\n> \toptions->pickaxe_opts |= DIFF_PICKAXE_KIND_G;\n> +\tif (arg && !*arg)\n> +\t\treturn error(_(\"-G requires a non-empty argument\"));\n> \treturn 0;\n> }\n> \n> @@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt,\n> \tBUG_ON_OPT_NEG(unset);\n> \toptions->pickaxe = arg;\n> \toptions->pickaxe_opts |= DIFF_PICKAXE_KIND_S;\n> +\tif (arg && !*arg)\n> +\t\treturn error(_(\"-S requires a non-empty argument\"));\n> \treturn 0;\n> }\n> \n> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh\n> index a675ace081..0e2f80a268 100755\n> --- a/t/t4209-log-pickaxe.sh\n> +++ b/t/t4209-log-pickaxe.sh\n> @@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'usage: -G and -S with empty argument' '\n> +\tcat >expect <<-\\EOF &&\n> +\terror: -S requires a non-empty argument\n> +\tEOF\n> +\n> +\ttest_expect_code 129 git log -S \"\" 2>actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\terror: -G requires a non-empty argument\n> +\tEOF\n> +\n> +\ttest_expect_code 129 git log -G \"\" 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_log\texpect_initial\t--grep initial\n> test_log\texpect_nomatch\t--grep InItial\n> test_log_icase\texpect_initial\t--grep InItial\n"},{"id":"512636","messageId":"Z7TffynGALJM4KfH@tapette.crustytoothpaste.net","threadId":"62960","inReplyTo":"xmqqbjuzxgn3.fsf@gitster.g","subject":"Re: [PATCH] diff: don't crash with empty argument to -G or -S","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-02-18T19:29:03Z","receivedAt":"2025-02-18T19:29:06Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-02-18 at 18:16:32, Junio C Hamano wrote:\n> I agree BUG is unwelcome. I am not sure about the value of\n> forbidding an empty string (I am sure about forbidding NULL,\n> though). \n> \n> If an empty matches everything, \"git log -S\" would skip changes that\n> would keep the number of lines, right? For the history of a project\n> that keeps track of source code, such a \"feature\" would not be\n> useful, but I can see a complaint by somebody who may want to keep\n> track of a \"list of things\" one-item-per-line, if we had been\n> allowing an empty string. It would be a regression for such a niche\n> user.\n\nI actually just ran a `git grep -e ''` to see what it does, and it\ndoes indeed match every line, so presumably `git log -G` would do so as\nwell.\n\nI do see your argument that this could be useful for a limited number of\nuse cases, but as someone who often keeps track of lists of things in\ntext files and therefore could be a target for that feature, I still\nfeel like this would be very much a corner case.\n\n> Luckily, since we have stopped with a \"BUG\", we do not have to worry\n> about backward compatibility in this case ;-)\n\nI agree. The good news is that we haven't broken anyone's workflow,\nunless their workflow involves trying to trigger bugs.\n\n> So I'd say that it may be a bit premature for us to declare\n> \"anything useful\", I am perfectly fine with the patch given here.\n> If somebody who wants to maintain a text file, one-item-per-line\n> that keeps track of a list of things to omit commits that do not\n> change the number of items, they can drop \"&& !*arg\" part, tweak the\n> message and add their own tests, once this fix lands and the dust\n> settles.\n\nExactly. If there's one thing I've learned, it's that there are lots of\nusers who will try new things, and I'm sure we'll get a report here or\nelsewhere that they'd like to add this feature if there's actually\ninterest. Fortunately, I expect that it shouldn't be too hard to add\nsuch a feature.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"}]}