Crash on empty pickaxe argument

Crash on empty pickaxe argument

[Jared Van Bortel]

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
<snip>
$ 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=<optimized out>, line=<optimized out>, 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=<optimized out>, line=<optimized out>, 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=<optimized out>) 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=<optimized out>) at builtin/log.c:950
#15 0x000055555555e56c in run_builtin (p=0x555555977f40 <commands.lto_priv+1440>, argc=<optimized out>, argv=<optimized out>, repo=0x555555986d40 <the_repo.lto_priv>)
    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=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.48.1/git.c:947
#19 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.48.1/common-main.c:64


Thanks,
Jared

Re: Crash on empty pickaxe argument

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

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
> <snip>
> $ 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH] diff: don't crash with empty argument to -G or -S

[brian m. carlson]

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 <cebtenzzre@gmail.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 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

Re: [PATCH] diff: don't crash with empty argument to -G or -S

[Elijah Newren]

On Mon, Feb 17, 2025 at 9:58 AM brian m. carlson
<sandals@crustytoothpaste.net> 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 <cebtenzzre@gmail.com>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  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.

Re: [PATCH] diff: don't crash with empty argument to -G or -S

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> 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 <cebtenzzre@gmail.com>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  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

Re: [PATCH] diff: don't crash with empty argument to -G or -S

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]