# Crash on empty pickaxe argument 6 messages from 2025-02-17 to 2025-02-18. Participants: Jared Van Bortel, brian m. carlson, Elijah Newren, Junio C Hamano. Thread: https://gitlist.dev/t/62960 ## Jared Van Bortel, 2025-02-17 01:24 Subject: Crash on empty pickaxe argument Message-ID: URL: https://gitlist.dev/e/b59b75a1ad77f59cc8d81e2dd5cc9607e53b50af.camel%40gmail.com ``` Hi all, I fat-fingered my enter key today and noticed this crash: $ git --version git version 2.48.1 $ mkdir foo && cd foo $ git init $ git commit --allow-empty -m "foo" [master (root-commit) ed4e080] foo $ git log -S "" BUG: diffcore-pickaxe.c:241: should have needle under -G or -S [1] 65421 IOT instruction (core dumped) git log -S "" $ git log -G "" BUG: diffcore-pickaxe.c:241: should have needle under -G or -S [1] 65457 IOT instruction (core dumped) git log -G "" Well, it says BUG right there. It probably shouldn't crash. Relevant backtrace for -S: #3 0x00007ffff7ce7582 in __GI_abort () at abort.c:73 #4 0x00005555558689aa in BUG_vfl (file=, line=, fmt=0x5555558ed498 "should have needle under -G or -S", params=0x7fffffffbf70) at /usr/src/debug/git/git-2.48.1/usage.c:318 #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 #6 0x000055555571527f in diffcore_pickaxe (o=0x7fffffffca90) at /usr/src/debug/git/git-2.48.1/diffcore-pickaxe.c:241 #7 diffcore_std (options=0x7fffffffca90) at /usr/src/debug/git/git-2.48.1/diff.c:7079 #8 0x00005555558b2788 in log_tree_diff_flush.isra.0 (opt=0x7fffffffc4d0) at /usr/src/debug/git/git-2.48.1/log-tree.c:929 #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 #10 log_tree_commit (opt=0x7fffffffc4d0, commit=0x5555559b7b80) at /usr/src/debug/git/git-2.48.1/log-tree.c:1188 #11 0x00005555555e68c8 in cmd_log_walk_no_free (rev=) at builtin/log.c:528 #12 cmd_log_walk_no_free (rev=rev@entry=0x7fffffffc4d0) at builtin/log.c:506 #13 0x00005555555e74bf in cmd_log_walk (rev=0x7fffffffc4d0) at builtin/log.c:569 #14 cmd_log (argc=3, argv=0x5555559b0790, prefix=0x0, repo=) at builtin/log.c:950 #15 0x000055555555e56c in run_builtin (p=0x555555977f40 , argc=, argv=, repo=0x555555986d40 ) at /usr/src/debug/git/git-2.48.1/git.c:480 #16 handle_builtin (args=args@entry=0x7fffffffd4a0) at /usr/src/debug/git/git-2.48.1/git.c:740 #17 0x000055555555ec34 in run_argv (args=0x7fffffffd4a0) at /usr/src/debug/git/git-2.48.1/git.c:807 #18 0x00005555555598a1 in cmd_main (argc=, argv=) at /usr/src/debug/git/git-2.48.1/git.c:947 #19 main (argc=, argv=) at /usr/src/debug/git/git-2.48.1/common-main.c:64 Thanks, Jared ``` ## brian m. carlson, 2025-02-17 01:58 Subject: Re: Crash on empty pickaxe argument Message-ID: URL: https://gitlist.dev/e/Z7KX2y-dXaZuPrW4%40tapette.crustytoothpaste.net In-Reply-To: ``` On 2025-02-17 at 01:24:14, Jared Van Bortel wrote: > Hi all, Hi, > I fat-fingered my enter key today and noticed this crash: > > $ git --version > git version 2.48.1 > $ mkdir foo && cd foo > $ git init > > $ git commit --allow-empty -m "foo" > [master (root-commit) ed4e080] foo > $ git log -S "" > BUG: diffcore-pickaxe.c:241: should have needle under -G or -S > [1] 65421 IOT instruction (core dumped) git log -S "" > $ git log -G "" > BUG: diffcore-pickaxe.c:241: should have needle under -G or -S > [1] 65457 IOT instruction (core dumped) git log -G "" > > > Well, it says BUG right there. It probably shouldn't crash. Relevant > backtrace for -S: Thanks for the report. I agree this is a bug and we shouldn't crash. I think the right thing to do here is reject empty arguments since those aren't actually very useful, so I'm in the process of writing a patch to do that now, which I'll send out either later this evening or tomorrow, depending on when CI finishes running. -- brian m. carlson (they/them or he/him) Toronto, Ontario, CA ``` ## brian m. carlson, 2025-02-17 17:57 Subject: [PATCH] diff: don't crash with empty argument to -G or -S Message-ID: <20250217175759.1576684-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20250217175759.1576684-1-sandals%40crustytoothpaste.net In-Reply-To: ``` The pickaxe options, -G and -S, need either a regex or a string to look through the history for. An empty value isn't very useful since it would either match everything or nothing, and what's worse, we presently crash with a BUG like so when the user provides one: BUG: diffcore-pickaxe.c:241: should have needle under -G or -S Since it's not very nice of us to crash and this wouldn't do anything useful anyway, let's simply inform the user that they must provide a non-empty argument and exit with an error if they provide an empty one instead. Reported-by: Jared Van Bortel Signed-off-by: brian m. carlson --- diff.c | 4 ++++ t/t4209-log-pickaxe.sh | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/diff.c b/diff.c index 019fb893a7..c89c15d98e 100644 --- a/diff.c +++ b/diff.c @@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt, BUG_ON_OPT_NEG(unset); options->pickaxe = arg; options->pickaxe_opts |= DIFF_PICKAXE_KIND_G; + if (arg && !*arg) + return error(_("-G requires a non-empty argument")); return 0; } @@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt, BUG_ON_OPT_NEG(unset); options->pickaxe = arg; options->pickaxe_opts |= DIFF_PICKAXE_KIND_S; + if (arg && !*arg) + return error(_("-S requires a non-empty argument")); return 0; } diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh index a675ace081..0e2f80a268 100755 --- a/t/t4209-log-pickaxe.sh +++ b/t/t4209-log-pickaxe.sh @@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' ' test_cmp expect actual ' +test_expect_success 'usage: -G and -S with empty argument' ' + cat >expect <<-\EOF && + error: -S requires a non-empty argument + EOF + + test_expect_code 129 git log -S "" 2>actual && + test_cmp expect actual && + + cat >expect <<-\EOF && + error: -G requires a non-empty argument + EOF + + test_expect_code 129 git log -G "" 2>actual && + test_cmp expect actual +' + test_log expect_initial --grep initial test_log expect_nomatch --grep InItial test_log_icase expect_initial --grep InItial ``` ## Elijah Newren, 2025-02-17 22:18 Subject: Re: [PATCH] diff: don't crash with empty argument to -G or -S Message-ID: URL: https://gitlist.dev/e/CABPp-BF01SYAr8TNxMBNvxPKseSqLH4mVC%3DMrrVp8w9ejEzK1g%40mail.gmail.com In-Reply-To: <20250217175759.1576684-1-sandals@crustytoothpaste.net> ``` On Mon, Feb 17, 2025 at 9:58 AM brian m. carlson wrote: > > The pickaxe options, -G and -S, need either a regex or a string to look > through the history for. An empty value isn't very useful since it > would either match everything or nothing, and what's worse, we presently > crash with a BUG like so when the user provides one: > > BUG: diffcore-pickaxe.c:241: should have needle under -G or -S > > Since it's not very nice of us to crash and this wouldn't do anything > useful anyway, let's simply inform the user that they must provide a > non-empty argument and exit with an error if they provide an empty one > instead. Makes sense. > Reported-by: Jared Van Bortel > Signed-off-by: brian m. carlson > --- > diff.c | 4 ++++ > t/t4209-log-pickaxe.sh | 16 ++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/diff.c b/diff.c > index 019fb893a7..c89c15d98e 100644 > --- a/diff.c > +++ b/diff.c > @@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt, > BUG_ON_OPT_NEG(unset); > options->pickaxe = arg; > options->pickaxe_opts |= DIFF_PICKAXE_KIND_G; > + if (arg && !*arg) > + return error(_("-G requires a non-empty argument")); > return 0; > } > > @@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt, > BUG_ON_OPT_NEG(unset); > options->pickaxe = arg; > options->pickaxe_opts |= DIFF_PICKAXE_KIND_S; > + if (arg && !*arg) > + return error(_("-S requires a non-empty argument")); > return 0; > } Simple fix, as expected. > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh > index a675ace081..0e2f80a268 100755 > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' ' > test_cmp expect actual > ' > > +test_expect_success 'usage: -G and -S with empty argument' ' > + cat >expect <<-\EOF && > + error: -S requires a non-empty argument > + EOF > + > + test_expect_code 129 git log -S "" 2>actual && > + test_cmp expect actual && > + > + cat >expect <<-\EOF && > + error: -G requires a non-empty argument > + EOF > + > + test_expect_code 129 git log -G "" 2>actual && > + test_cmp expect actual > +' Looks good to me. ``` ## Junio C Hamano, 2025-02-18 18:16 Subject: Re: [PATCH] diff: don't crash with empty argument to -G or -S Message-ID: URL: https://gitlist.dev/e/xmqqbjuzxgn3.fsf%40gitster.g In-Reply-To: <20250217175759.1576684-1-sandals@crustytoothpaste.net> ``` "brian m. carlson" writes: > The pickaxe options, -G and -S, need either a regex or a string to look > through the history for. An empty value isn't very useful since it > would either match everything or nothing, and what's worse, we presently > crash with a BUG like so when the user provides one: > > BUG: diffcore-pickaxe.c:241: should have needle under -G or -S I agree BUG is unwelcome. I am not sure about the value of forbidding an empty string (I am sure about forbidding NULL, though). If an empty matches everything, "git log -S" would skip changes that would keep the number of lines, right? For the history of a project that keeps track of source code, such a "feature" would not be useful, but I can see a complaint by somebody who may want to keep track of a "list of things" one-item-per-line, if we had been allowing an empty string. It would be a regression for such a niche user. Luckily, since we have stopped with a "BUG", we do not have to worry about backward compatibility in this case ;-) > Since it's not very nice of us to crash and this wouldn't do anything > useful anyway, let's simply inform the user that they must provide a > non-empty argument and exit with an error if they provide an empty one > instead. So I'd say that it may be a bit premature for us to declare "anything useful", I am perfectly fine with the patch given here. If somebody who wants to maintain a text file, one-item-per-line that keeps track of a list of things to omit commits that do not change the number of items, they can drop "&& !*arg" part, tweak the message and add their own tests, once this fix lands and the dust settles. Thanks for a quick fix. Will queue. > > Reported-by: Jared Van Bortel > Signed-off-by: brian m. carlson > --- > diff.c | 4 ++++ > t/t4209-log-pickaxe.sh | 16 ++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/diff.c b/diff.c > index 019fb893a7..c89c15d98e 100644 > --- a/diff.c > +++ b/diff.c > @@ -5493,6 +5493,8 @@ static int diff_opt_pickaxe_regex(const struct option *opt, > BUG_ON_OPT_NEG(unset); > options->pickaxe = arg; > options->pickaxe_opts |= DIFF_PICKAXE_KIND_G; > + if (arg && !*arg) > + return error(_("-G requires a non-empty argument")); > return 0; > } > > @@ -5504,6 +5506,8 @@ static int diff_opt_pickaxe_string(const struct option *opt, > BUG_ON_OPT_NEG(unset); > options->pickaxe = arg; > options->pickaxe_opts |= DIFF_PICKAXE_KIND_S; > + if (arg && !*arg) > + return error(_("-S requires a non-empty argument")); > return 0; > } > > diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh > index a675ace081..0e2f80a268 100755 > --- a/t/t4209-log-pickaxe.sh > +++ b/t/t4209-log-pickaxe.sh > @@ -93,6 +93,22 @@ test_expect_success 'usage: --no-pickaxe-regex' ' > test_cmp expect actual > ' > > +test_expect_success 'usage: -G and -S with empty argument' ' > + cat >expect <<-\EOF && > + error: -S requires a non-empty argument > + EOF > + > + test_expect_code 129 git log -S "" 2>actual && > + test_cmp expect actual && > + > + cat >expect <<-\EOF && > + error: -G requires a non-empty argument > + EOF > + > + test_expect_code 129 git log -G "" 2>actual && > + test_cmp expect actual > +' > + > test_log expect_initial --grep initial > test_log expect_nomatch --grep InItial > test_log_icase expect_initial --grep InItial ``` ## brian m. carlson, 2025-02-18 19:29 Subject: Re: [PATCH] diff: don't crash with empty argument to -G or -S Message-ID: URL: https://gitlist.dev/e/Z7TffynGALJM4KfH%40tapette.crustytoothpaste.net In-Reply-To: ``` On 2025-02-18 at 18:16:32, Junio C Hamano wrote: > I agree BUG is unwelcome. I am not sure about the value of > forbidding an empty string (I am sure about forbidding NULL, > though). > > If an empty matches everything, "git log -S" would skip changes that > would keep the number of lines, right? For the history of a project > that keeps track of source code, such a "feature" would not be > useful, but I can see a complaint by somebody who may want to keep > track of a "list of things" one-item-per-line, if we had been > allowing an empty string. It would be a regression for such a niche > user. I actually just ran a `git grep -e ''` to see what it does, and it does indeed match every line, so presumably `git log -G` would do so as well. I do see your argument that this could be useful for a limited number of use cases, but as someone who often keeps track of lists of things in text files and therefore could be a target for that feature, I still feel like this would be very much a corner case. > Luckily, since we have stopped with a "BUG", we do not have to worry > about backward compatibility in this case ;-) I agree. The good news is that we haven't broken anyone's workflow, unless their workflow involves trying to trigger bugs. > So I'd say that it may be a bit premature for us to declare > "anything useful", I am perfectly fine with the patch given here. > If somebody who wants to maintain a text file, one-item-per-line > that keeps track of a list of things to omit commits that do not > change the number of items, they can drop "&& !*arg" part, tweak the > message and add their own tests, once this fix lands and the dust > settles. Exactly. If there's one thing I've learned, it's that there are lots of users who will try new things, and I'm sure we'll get a report here or elsewhere that they'd like to add this feature if there's actually interest. Fortunately, I expect that it shouldn't be too hard to add such a feature. -- brian m. carlson (they/them or he/him) Toronto, Ontario, CA ```