[BUG] git crashes on simple rev-parse incantation

[BUG] git crashes on simple rev-parse incantation

[Ingy dot Net]

$ git --version
git version 2.34.1
$ uname -a
Linux zed 5.15.0-48-generic #54-Ubuntu SMP Fri Aug 26 13:26:29 UTC
2022 x86_64 x86_64 x86_64 GNU/Linux

I get:

$ git rev-parse --parseopt -- <<<$'x\n--\n=, x\n'
fatal: Out of memory, malloc failed (tried to allocate
18446744073709551615 bytes)

----
Here's a less cryptic that fails the same way:

OPTS_SPEC="\
some-command [<options>] <args>...

some-command does foo and bar!
--
h,help    show the help
=,equal   nooooooooooooo!
"

echo "$OPTS_SPEC" |
  git rev-parse --parseopt --

----
'=' as a short form in the opts spec seems to be the culprit.

I was trying to see what short options could be parsed like '-9',
'-_', '-+', etc.

In addition to '=', '!' and '*' also cause crashes.
I tested all the other ascii punctuation and didn't see crashes.

Re: [BUG] git crashes on simple rev-parse incantation

[Øystein Walle]

FWIW, I can reproduce on master and next.

Øsse

[PATCH] rev-parse: Detect missing opt-spec

[Øystein Walle]

If a line in parseopts's input starts with one of the flag characters it
is erroneously parsed as a opt-spec where the short name of the option
is the flag character itself and the long name is after the end of the
string. This makes Git want to allocate SIZE_MAX bytes of memory at this
line:

    o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);

Since s and sb.buf are equal the second argument is -2 (except unsigned)
and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.

Avoid this by checking whether a flag character was found in the zeroth
position.

Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
 builtin/rev-parse.c           | 3 +++
 t/t1502-rev-parse-parseopt.sh | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index b259d8990a..04958cf9a9 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		if (!s)
 			s = help;
 
+		if (s == sb.buf)
+			die(_("Missing opt-spec before option flags"));
+
 		if (s - sb.buf == 1) /* short option only */
 			o->short_name = *sb.buf;
 		else if (sb.buf[1] != ',') /* long option only */
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 284fe18e72..15bc240027 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	test_cmp expect actual
 '
 
+test_expect_success 'test --parseopt invalid opt-spec' '
+	test_write_lines x -- "=, x" >spec &&
+	echo "fatal: Missing opt-spec before option flags" >expect &&
+	test_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
 	sed -e "s/^|//" >spec <<-\EOF &&
 	|cmd [--some-option]
@@ -337,3 +344,5 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 '
 
 test_done
+
+test_done
-- 
2.34.1

Re: [PATCH] rev-parse: Detect missing opt-spec

[Eric Sunshine]

On Fri, Sep 2, 2022 at 1:10 AM Øystein Walle <oystwa@gmail.com> wrote:
> If a line in parseopts's input starts with one of the flag characters it
> is erroneously parsed as a opt-spec where the short name of the option
> is the flag character itself and the long name is after the end of the
> string. This makes Git want to allocate SIZE_MAX bytes of memory at this
> line:
>
>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
>
> Since s and sb.buf are equal the second argument is -2 (except unsigned)
> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.
>
> Avoid this by checking whether a flag character was found in the zeroth
> position.
>
> Signed-off-by: Øystein Walle <oystwa@gmail.com>

Perhaps add a:

    Reported-by: Ingy dot Net <ingy@ingy.net>

trailer?

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
> +               if (s == sb.buf)
> +                       die(_("Missing opt-spec before option flags"));

There is a bit of a mix in this file already, but these days, we tend
to start error messages with lowercase:

    die(_("missing opt-spec before option flags"));

> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
> +test_expect_success 'test --parseopt invalid opt-spec' '
> +       test_write_lines x -- "=, x" >spec &&
> +       echo "fatal: Missing opt-spec before option flags" >expect &&
> +       test_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&
> +       test_cmp expect actual
> +'
> +
> @@ -337,3 +344,5 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
>
>  test_done
> +
> +test_done

Um? Debugging leftover?

[PATCH v2] rev-parse: Detect missing opt-spec

[Øystein Walle]

If a line in parseopts's input starts with one of the flag characters it
is erroneously parsed as a opt-spec where the short name of the option
is the flag character itself and the long name is after the end of the
string. This makes Git want to allocate SIZE_MAX bytes of memory at this
line:

    o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);

Since s and sb.buf are equal the second argument is -2 (except unsigned)
and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.

Avoid this by checking whether a flag character was found in the zeroth
position.

Reported-by: Ingy dot Net <ingy@ingy.net>
Signed-off-by: Øystein Walle <oystwa@gmail.com>
---

Thanks for the review, Eric (should I then add a Reviewed-by trailer?).
Fixed the casing, added the suggested trailer, and remove the
superfluous test_done which indeed was a leftover. 

 builtin/rev-parse.c           | 3 +++
 t/t1502-rev-parse-parseopt.sh | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index b259d8990a..85c271acd7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		if (!s)
 			s = help;
 
+		if (s == sb.buf)
+			die(_("missing opt-spec before option flags"));
+
 		if (s - sb.buf == 1) /* short option only */
 			o->short_name = *sb.buf;
 		else if (sb.buf[1] != ',') /* long option only */
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 284fe18e72..75f576249c 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	test_cmp expect actual
 '
 
+test_expect_success 'test --parseopt invalid opt-spec' '
+	test_write_lines x -- "=, x" >spec &&
+	echo "fatal: Missing opt-spec before option flags" >expect &&
+	test_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
 	sed -e "s/^|//" >spec <<-\EOF &&
 	|cmd [--some-option]
-- 
2.34.1

Re: [PATCH v2] rev-parse: Detect missing opt-spec

[Eric Sunshine]

On Fri, Sep 2, 2022 at 2:40 AM Øystein Walle <oystwa@gmail.com> wrote:
> If a line in parseopts's input starts with one of the flag characters it
> is erroneously parsed as a opt-spec where the short name of the option
> is the flag character itself and the long name is after the end of the
> string. This makes Git want to allocate SIZE_MAX bytes of memory at this
> line:
>
>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
>
> Since s and sb.buf are equal the second argument is -2 (except unsigned)
> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.
>
> Avoid this by checking whether a flag character was found in the zeroth
> position.
>
> Reported-by: Ingy dot Net <ingy@ingy.net>
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
>
> Thanks for the review, Eric (should I then add a Reviewed-by trailer?).
> Fixed the casing, added the suggested trailer, and remove the
> superfluous test_done which indeed was a leftover.

Thanks for addressing my minor comments.

Since I only scanned my eye over the commit message and patch text,
but didn't actually dig into the code to verify if the fix was
correct, a Reviewed-by: would be misleading, so let's not add that
trailer.

Re: [PATCH] rev-parse: Detect missing opt-spec

[SZEDER Gábor]

On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:
> If a line in parseopts's input starts with one of the flag characters it
> is erroneously parsed as a opt-spec where the short name of the option
> is the flag character itself and the long name is after the end of the
> string. This makes Git want to allocate SIZE_MAX bytes of memory at this
> line:
> 
>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
> 
> Since s and sb.buf are equal the second argument is -2 (except unsigned)
> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.

I suspect (but didn't actually check) that this bug was added in
2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,
2015-07-14).

> Avoid this by checking whether a flag character was found in the zeroth
> position.
> 
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
>  builtin/rev-parse.c           | 3 +++
>  t/t1502-rev-parse-parseopt.sh | 9 +++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index b259d8990a..04958cf9a9 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  		if (!s)
>  			s = help;
>  
> +		if (s == sb.buf)
> +			die(_("Missing opt-spec before option flags"));
> +
>  		if (s - sb.buf == 1) /* short option only */
>  			o->short_name = *sb.buf;
>  		else if (sb.buf[1] != ',') /* long option only */
> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index 284fe18e72..15bc240027 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'test --parseopt invalid opt-spec' '
> +	test_write_lines x -- "=, x" >spec &&
> +	echo "fatal: Missing opt-spec before option flags" >expect &&
> +	test_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&

When checking an error message please don't look for it on standard
output; i.e. the redirection at the end should be '2>actual', or
perheps even better '2>err'.

> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
>  	sed -e "s/^|//" >spec <<-\EOF &&
>  	|cmd [--some-option]
> @@ -337,3 +344,5 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
>  '
>  
>  test_done
> +
> +test_done
> -- 
> 2.34.1
> 

Re: [PATCH] rev-parse: Detect missing opt-spec

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:
>> If a line in parseopts's input starts with one of the flag characters it
>> is erroneously parsed as a opt-spec where the short name of the option
>> is the flag character itself and the long name is after the end of the
>> string. This makes Git want to allocate SIZE_MAX bytes of memory at this
>> line:
>> 
>>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
>> 
>> Since s and sb.buf are equal the second argument is -2 (except unsigned)
>> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.
>
> I suspect (but didn't actually check) that this bug was added in
> 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,
> 2015-07-14).

Good thing to add to the proposed log message.  Thanks.

Also, Øystein "Detect" -> "detect" on the title (you can see the
convention in the output from "git shortlog --no-merges").

>>  		if (!s)
>>  			s = help;
>>  
>> +		if (s == sb.buf)
>> +			die(_("Missing opt-spec before option flags"));
>> +

OK.

>> +test_expect_success 'test --parseopt invalid opt-spec' '
>> +	test_write_lines x -- "=, x" >spec &&
>> +	echo "fatal: Missing opt-spec before option flags" >expect &&
>> +	test_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&
>
> When checking an error message please don't look for it on standard
> output; i.e. the redirection at the end should be '2>actual', or
> perheps even better '2>err'.

Again, very good point.

Thanks.

[PATCH] rev-parse --parseopt: detect missing opt-spec

[Øystein Walle]

After 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,
2015-07-14) updated the parser, a line in parseopts's input can start
with one of the flag characters and be erroneously parsed as a opt-spec
where the short name of the option is the flag character itself and the
long name is after the end of the string. This makes Git want to
allocate SIZE_MAX bytes of memory at this line:

    o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);

Since s and sb.buf are equal the second argument is -2 (except unsigned)
and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.

Avoid this by checking whether a flag character was found in the zeroth
position.

Reported-by: Ingy dot Net <ingy@ingy.net>
Reviewed-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Øystein Walle <oystwa@gmail.com>
---

Hi guys, thanks for the review. I incorporated a reference to the old
commit into the message (and took the liberty of adding --parseopt to
the subject like it had). I tried to verify that it was in fact this
commit, since the code prior to this one had the exact same xmemdupz()
call. I wasn't able to build that commit, and reverting it also wasn't
straightforward. But I'm fairly confident it's the case since the old
call had an if similar to the one added here.

I completely agree with the changes to the test; it makes little sense
to mix stdout and stderr here.

Jeff, I agree that perhaps a larger rewrite would be better. I
personally can get easily confused by "sporadic" ifs like this one in
the middle of a piece of code. At least in this case the message within
die() neatly explains what's going on.

Øsse

 builtin/rev-parse.c           | 3 +++
 t/t1502-rev-parse-parseopt.sh | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index b259d8990a..85c271acd7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		if (!s)
 			s = help;
 
+		if (s == sb.buf)
+			die(_("missing opt-spec before option flags"));
+
 		if (s - sb.buf == 1) /* short option only */
 			o->short_name = *sb.buf;
 		else if (sb.buf[1] != ',') /* long option only */
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 284fe18e72..de1d48f3ba 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	test_cmp expect actual
 '
 
+test_expect_success 'test --parseopt invalid opt-spec' '
+	test_write_lines x -- "=, x" >spec &&
+	echo "fatal: missing opt-spec before option flags" >expect &&
+	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
+	test_cmp expect err
+'
+
 test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
 	sed -e "s/^|//" >spec <<-\EOF &&
 	|cmd [--some-option]
-- 
2.34.1

[PATCH] rev-parse --parseopt: detect missing opt-spec

[Øystein Walle]

Damnit, forgot the v3. Hope that's okay.

Øsse

Re: [PATCH] rev-parse --parseopt: detect missing opt-spec

[Junio C Hamano]

Øystein Walle <oystwa@gmail.com> writes:

> Damnit, forgot the v3. Hope that's okay.
>
> Øsse

Sure, and thanks.

Re: [PATCH] rev-parse --parseopt: detect missing opt-spec

[SZEDER Gábor]

On Fri, Sep 02, 2022 at 07:59:02PM +0200, Øystein Walle wrote:
> After 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,
> 2015-07-14)

Oh, no, that's not the first bad commit!  The segfault started
earlier, with commit 9bab5b6061 (rev-parse --parseopt: option argument
name hints, 2014-03-22).

Before that it printed the following for the spec input used in the
new test:

  $ ./git rev-parse --parseopt -- <spec
  set -- --

I'm not sure what the desired behavior should have been back then.

Anyway, since 2d893dff4c the documentation is clear that 'opt-spec'
"May not contain any of the `<flags>` characters", so now erroring out
is the right thing to do.

> updated the parser, a line in parseopts's input can start
> with one of the flag characters and be erroneously parsed as a opt-spec
> where the short name of the option is the flag character itself and the
> long name is after the end of the string. This makes Git want to
> allocate SIZE_MAX bytes of memory at this line:
> 
>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
> 
> Since s and sb.buf are equal the second argument is -2 (except unsigned)
> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.
> 
> Avoid this by checking whether a flag character was found in the zeroth
> position.
> 
> Reported-by: Ingy dot Net <ingy@ingy.net>
> Reviewed-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
> 
> Hi guys, thanks for the review. I incorporated a reference to the old
> commit into the message (and took the liberty of adding --parseopt to
> the subject like it had). I tried to verify that it was in fact this
> commit, since the code prior to this one had the exact same xmemdupz()
> call. I wasn't able to build that commit,

Yeah, that's why I didn't check it in the morning...

NO_OPENSSL=1 NO_PERL_MAKEMAKER=1 sometimes helps building older
versions on modern setups, and, FWIW, Ubuntu 16.04 can build both
today's Git and v1.6.0 (my goto version when I want to check
historical behavior) even with OPENSSL.

>  builtin/rev-parse.c           | 3 +++
>  t/t1502-rev-parse-parseopt.sh | 7 +++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index b259d8990a..85c271acd7 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  		if (!s)
>  			s = help;
>  
> +		if (s == sb.buf)
> +			die(_("missing opt-spec before option flags"));
> +
>  		if (s - sb.buf == 1) /* short option only */
>  			o->short_name = *sb.buf;
>  		else if (sb.buf[1] != ',') /* long option only */
> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index 284fe18e72..de1d48f3ba 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'test --parseopt invalid opt-spec' '
> +	test_write_lines x -- "=, x" >spec &&
> +	echo "fatal: missing opt-spec before option flags" >expect &&
> +	test_must_fail git rev-parse --parseopt -- >out <spec 2>err &&
> +	test_cmp expect err
> +'
> +
>  test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
>  	sed -e "s/^|//" >spec <<-\EOF &&
>  	|cmd [--some-option]
> -- 
> 2.34.1
> 

Re: [PATCH] rev-parse --parseopt: detect missing opt-spec

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> NO_OPENSSL=1 NO_PERL_MAKEMAKER=1 sometimes helps building older
> versions on modern setups, and, FWIW, Ubuntu 16.04 can build both
> today's Git and v1.6.0 (my goto version when I want to check
> historical behavior) even with OPENSSL.

Thanks for a good piece of advice.

Re: [PATCH] rev-parse: Detect missing opt-spec

[Jeff King]

On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index b259d8990a..04958cf9a9 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  		if (!s)
>  			s = help;
>  
> +		if (s == sb.buf)
> +			die(_("Missing opt-spec before option flags"));
> +
>  		if (s - sb.buf == 1) /* short option only */
>  			o->short_name = *sb.buf;
>  		else if (sb.buf[1] != ',') /* long option only */

I think this is the right thing to do, at least for now. Certainly it
catches the bug. It doesn't allow short or long option names to contain
any flag characters, but that's probably OK in practice.

I think one could make an argument that cmd_parseopt() should do a
better job of parsing left-to-right. The reason it missed this case is
that it calls strpbrk(), expecting to jump past the short/long option
names, but it jumps less far than expected.

If the parsing were more left-to-right, like:

  - start with pointer at beginning of sb.buf

  - look for acceptable character for short option, or "," for none;
    advance pointer if found, otherwise bail

  - look for syntactically valid long option name; advance pointer,
    otherwise bail

  - look for valid flags

then I think it becomes much easier to reason about what is valid for
each item. And we _could_ do things like allowing a short-option that is
also a flag-character, if we wanted to.

But IMHO such a refactoring can come later, or not at all. While it
might make the code a bit more clear, I don't think it meaningfully
improves the behavior. And either way, we should start with a minimal
and easy-to-verify fix like you have here.

-Peff