git column fails (or crashes) if padding is negative

git column fails (or crashes) if padding is negative

[Tiago Pascoal]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

Call git column with a negative padding value, e.g. `git column --padding -1`

If the number if bigger than -3, the command will fail with the following error message:

  Floating point exception

If the number is -5 or greater, the command will fail with the following error message:

  fatal: Out of memory, malloc failed (tried to allocate 18446744073709551615 bytes)

eg:
$ seq 1 100 | git column --mode=column  --padding=-5
fatal: Out of memory, malloc failed (tried to allocate 18446744073709551615 bytes)

What did you expect to happen? (Expected behavior)

An error message indicating that the padding value is invalid and not a crash.

What happened instead? (Actual behavior)

Failed command or even an OOM error depending on the padding value.

What's different between what you expected and what actually happened?

Anything else you want to add:


[System Info]
git version:
git version 2.34.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]
not run from a git repository - no hooks to show

Re: git column fails (or crashes) if padding is negative

[Kristoffer Haugsbakk]

Hi

I wasn’t able to reproduce quite what you got but kind of the same.

```
$ seq 1 24 | git column --mode=column --padding=-1
12345678910<binary?><numbers>
$ seq 1 24 | git column --mode=column --padding=-3
fatal: Data too large to fit into virtual memory space.
$ seq 1 24 | git column --mode=column --padding=-5
fatal: Out of memory, malloc failed (tried to allocate 18446744073709551614 bytes)
```

This is an “Internal helper command” under the “plumbing” suite. And I
get the impression that sometimes these fallthroughs are treated as
“don’t do that”. But I don’t know.

On the other hand it failing inside malloc looks weird. Why not catch
this before the malloc call is made?

[System Info]
git version:
git version 2.43.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-17-generic #17~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Jan 16 14:32:32 UTC 2 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]


-- 
Kristoffer Haugsbakk

[PATCH] column: disallow negative padding

[Kristoffer Haugsbakk]

A negative padding can cause some problems in the memory allocator:

• floating point exception
• data too large to fit into virtual memory space
• OOM

Disallow negative padding. Reuse a translation string from
`fast-import`.

Reported-by: Tiago Pascoal <tiago@pascoal.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/column.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/column.c b/builtin/column.c
index e80218f81f9..82902d149c2 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
 	memset(&copts, 0, sizeof(copts));
 	copts.padding = 1;
 	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
+	if (copts.padding < 0)
+		die("%s: argument must be a non-negative integer", "padding");
 	if (argc)
 		usage_with_options(builtin_column_usage, options);
 	if (real_command || command) {
-- 
2.43.0

Re: git column fails (or crashes) if padding is negative

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> ```
> $ seq 1 24 | git column --mode=column --padding=-1
> 12345678910<binary?><numbers>
> $ seq 1 24 | git column --mode=column --padding=-3
> fatal: Data too large to fit into virtual memory space.
> $ seq 1 24 | git column --mode=column --padding=-5
> fatal: Out of memory, malloc failed (tried to allocate 18446744073709551614 bytes)
> ```
>
> This is an “Internal helper command” under the “plumbing” suite. And I
> get the impression that sometimes these fallthroughs are treated as
> “don’t do that”. But I don’t know.

If the nonsense input is easy to tell, then telling "don't feed
nonsense input" to the user while rejecting such nonsense input
would be a good idea.

> On the other hand it failing inside malloc looks weird. Why not catch
> this before the malloc call is made?

Presumably, the parameter we prepare before calling malloc() is of
unsigned type, and feeding a negative value to such a callchain
would cast it to a large unsigned value?

Indeed, whereever cops.padding is referenced in column.c, it clearly
is assumed that it is a non-negative value.  *width accumulates the
width of data items plus padding, and it also is used to divide some
number to arrive at the number of columns, so by tweaking the padding
to the right value, you probably should be able to cause division by
zero, too, in column.c:layout().

Hopefully the attached would be a good place to start (I am not
going to finish it with log message, tests, and fixes to other
places).

 builtin/column.c | 2 ++
 column.c         | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git c/builtin/column.c w/builtin/column.c
index e80218f81f..8537d09d2b 100644
--- c/builtin/column.c
+++ w/builtin/column.c
@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
 	memset(&copts, 0, sizeof(copts));
 	copts.padding = 1;
 	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
+	if (copts.padding < 0)
+		die(_("--padding must be non-negative"));
 	if (argc)
 		usage_with_options(builtin_column_usage, options);
 	if (real_command || command) {
diff --git c/column.c w/column.c
index ff2f0abf39..9cc703832a 100644
--- c/column.c
+++ w/column.c
@@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,
 	memset(&nopts, 0, sizeof(nopts));
 	nopts.indent = opts && opts->indent ? opts->indent : "";
 	nopts.nl = opts && opts->nl ? opts->nl : "\n";
-	nopts.padding = opts ? opts->padding : 1;
+	nopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;
 	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
 	if (!column_active(colopts)) {
 		display_plain(list, "", "\n");
@@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
 		strvec_pushf(argv, "--width=%d", opts->width);
 	if (opts && opts->indent)
 		strvec_pushf(argv, "--indent=%s", opts->indent);
-	if (opts && opts->padding)
+	if (opts && 0 < opts->padding)
 		strvec_pushf(argv, "--padding=%d", opts->padding);
 
 	fflush(stdout);

Re: [PATCH] column: disallow negative padding

[Kristoffer Haugsbakk]

I forgot tests.

-- 
Kristoffer

Re: [PATCH] column: disallow negative padding

[Chris Torek]

On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> I forgot tests.

You presumably also wanted the `_` here for gettext-ing:

> +               die("%s: argument must be a non-negative integer", "padding");

Chris

Re: git column fails (or crashes) if padding is negative

[Kristoffer Haugsbakk]

On Fri, Feb 9, 2024, at 18:57, Junio C Hamano wrote:
>  builtin/column.c | 2 ++
>  column.c         | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> […]
> diff --git c/column.c w/column.c
> index ff2f0abf39..9cc703832a 100644
> --- c/column.c
> +++ w/column.c
> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list,
> unsigned int colopts,
>  	memset(&nopts, 0, sizeof(nopts));
>  	nopts.indent = opts && opts->indent ? opts->indent : "";
>  	nopts.nl = opts && opts->nl ? opts->nl : "\n";
> -	nopts.padding = opts ? opts->padding : 1;
> +	nopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;

If these two are meant to check the same condition as in
`builtin/column.c`, shouldn’t it be `0 <= opts->padding`?

>  	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
>  	if (!column_active(colopts)) {
>  		display_plain(list, "", "\n");
> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct
> column_options *opts)
>  		strvec_pushf(argv, "--width=%d", opts->width);
>  	if (opts && opts->indent)
>  		strvec_pushf(argv, "--indent=%s", opts->indent);
> -	if (opts && opts->padding)
> +	if (opts && 0 < opts->padding)
>  		strvec_pushf(argv, "--padding=%d", opts->padding);
>
>  	fflush(stdout);

-- 
Kristoffer Haugsbakk

Re: [PATCH] column: disallow negative padding

[Kristoffer Haugsbakk]

On Sat, Feb 10, 2024, at 10:48, Chris Torek wrote:
> On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
>> I forgot tests.
>
> You presumably also wanted the `_` here for gettext-ing:
>
>> +               die("%s: argument must be a non-negative integer", "padding");
>
> Chris

Yeah, thanks. You probably saved me a v3. :)

Although I failed to notice that the string I stole was just a plain
string, not a translation string. And apparently there are no generic
“non-negative” translation strings. So I’ll just make a new one.

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH] column: disallow negative padding

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Sat, Feb 10, 2024, at 10:48, Chris Torek wrote:
>> On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk
>> <code@khaugsbakk.name> wrote:
>>> I forgot tests.
>>
>> You presumably also wanted the `_` here for gettext-ing:
>>
>>> +               die("%s: argument must be a non-negative integer", "padding");
>>
>> Chris
>
> Yeah, thanks. You probably saved me a v3. :)
>
> Although I failed to notice that the string I stole was just a plain
> string, not a translation string. And apparently there are no generic
> “non-negative” translation strings. So I’ll just make a new one.

The last time I took a look, I thought there were more than just the
single entry point you patched that can feed negative padding into
the machinery?  Don't you need to cover them as well?

Thanks.

Re: [PATCH] column: disallow negative padding

[Kristoffer Haugsbakk]

On Sun, Feb 11, 2024, at 18:55, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> Yeah, thanks. You probably saved me a v3. :)
>>
>> Although I failed to notice that the string I stole was just a plain
>> string, not a translation string. And apparently there are no generic
>> “non-negative” translation strings. So I’ll just make a new one.
>
> The last time I took a look, I thought there were more than just the
> single entry point you patched that can feed negative padding into
> the machinery?  Don't you need to cover them as well?
>
> Thanks.

I’ve incorporated the `column.c` patch you posted in my
not-yet-published v2. Hopefully that was it.(? :) ) I’ll take another
look.

v2 is finished now so maybe I’ll send it out soon.

-- 
Kristoffer Haugsbakk

[PATCH v2] column: disallow negative padding

[Kristoffer Haugsbakk]

A negative padding does not make sense and can cause errors in the
memory allocator since it’s interpreted as an unsigned integer.

Disallow negative padding. Also guard against negative padding in
`column.c` where it is conditionally used.

Reported-by: Tiago Pascoal <tiago@pascoal.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    • Incorporate Junio’s changes (guard against negative padding in
      `column.c`)
    • Tweak commit message based on Junio’s analysis
    • Use gettext for error message
      • However I noticed that the “translation string” from `fast-import`
        isn’t a translation string. So let’s invent a new one and use a
        parameter so that it can be used elsewhere.
    • Make a test

 builtin/column.c  |  2 ++
 column.c          |  4 ++--
 t/t9002-column.sh | 11 +++++++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/column.c b/builtin/column.c
index e80218f81f9..10ff7e01668 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
 	memset(&copts, 0, sizeof(copts));
 	copts.padding = 1;
 	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
+	if (copts.padding < 0)
+		die(_("%s must be non-negative"), "--padding");
 	if (argc)
 		usage_with_options(builtin_column_usage, options);
 	if (real_command || command) {
diff --git a/column.c b/column.c
index ff2f0abf399..c723428bc70 100644
--- a/column.c
+++ b/column.c
@@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,
 	memset(&nopts, 0, sizeof(nopts));
 	nopts.indent = opts && opts->indent ? opts->indent : "";
 	nopts.nl = opts && opts->nl ? opts->nl : "\n";
-	nopts.padding = opts ? opts->padding : 1;
+	nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;
 	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
 	if (!column_active(colopts)) {
 		display_plain(list, "", "\n");
@@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
 		strvec_pushf(argv, "--width=%d", opts->width);
 	if (opts && opts->indent)
 		strvec_pushf(argv, "--indent=%s", opts->indent);
-	if (opts && opts->padding)
+	if (opts && 0 <= opts->padding)
 		strvec_pushf(argv, "--padding=%d", opts->padding);
 
 	fflush(stdout);
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 348cc406582..d5b98e615bc 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -196,4 +196,15 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'padding must be non-negative' '
+	cat >input <<\EOF &&
+1 2 3 4 5 6
+EOF
+	cat >expected <<\EOF &&
+fatal: --padding must be non-negative
+EOF
+	test_must_fail git column --mode=column --padding=-1 <input >actual 2>&1 &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.43.0

Re: [PATCH v2] column: disallow negative padding

[Rubén Justo]

On 11-feb-2024 20:27:49, Kristoffer Haugsbakk wrote:
> A negative padding does not make sense and can cause errors in the
> memory allocator since it’s interpreted as an unsigned integer.
> 
> Disallow negative padding. Also guard against negative padding in
> `column.c` where it is conditionally used.
> 
> Reported-by: Tiago Pascoal <tiago@pascoal.net>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> 
> Notes (series):
>     v2:
>     • Incorporate Junio’s changes (guard against negative padding in
>       `column.c`)
>     • Tweak commit message based on Junio’s analysis
>     • Use gettext for error message
>       • However I noticed that the “translation string” from `fast-import`
>         isn’t a translation string. So let’s invent a new one and use a
>         parameter so that it can be used elsewhere.
>     • Make a test
> 
>  builtin/column.c  |  2 ++
>  column.c          |  4 ++--
>  t/t9002-column.sh | 11 +++++++++++
>  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/column.c b/builtin/column.c
> index e80218f81f9..10ff7e01668 100644
> --- a/builtin/column.c
> +++ b/builtin/column.c
> @@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
>  	memset(&copts, 0, sizeof(copts));
>  	copts.padding = 1;
>  	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
> +	if (copts.padding < 0)
> +		die(_("%s must be non-negative"), "--padding");

We clearly inform the user and die.  No more OOM errors, or worse.
Good.

And the message avoids translation problems.  Excellent.

>  	if (argc)
>  		usage_with_options(builtin_column_usage, options);
>  	if (real_command || command) {
> diff --git a/column.c b/column.c
> index ff2f0abf399..c723428bc70 100644
> --- a/column.c
> +++ b/column.c
> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,
>  	memset(&nopts, 0, sizeof(nopts));
>  	nopts.indent = opts && opts->indent ? opts->indent : "";
>  	nopts.nl = opts && opts->nl ? opts->nl : "\n";
> -	nopts.padding = opts ? opts->padding : 1;
> +	nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;

This changes what Junio proposed.  Is this on purpose?

While we're here, I wonder if silently ignoring a negative value in
.padding is the right thing to do.

There are several callers of print_columns():

builtin/branch.c:           print_columns(&output, colopts, NULL);
builtin/clean.c:    print_columns(&list, colopts, &copts);
builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);
builtin/column.c:    print_columns(&list, colopts, &copts);
help.c:     print_columns(&list, colopts, &copts);
wt-status.c:       print_columns(&output, s->colopts, &copts);

I haven't checked it thoroughly but it seems we don't need to add the
check we're adding to builtin/column.c, to any of the other callers.
However, it is possible that these or other new callers may need it in
the future.  If so, we should consider doing something like:

diff --git a/column.c b/column.c
index c723428bc7..4f870c725f 100644
--- a/column.c
+++ b/column.c
@@ -186,6 +186,9 @@ void print_columns(const struct string_list *list, unsigned int colopts,
                return;
        assert((colopts & COL_ENABLE_MASK) != COL_AUTO);

+       if (opts && (0 <= opts->padding))
+               BUG("padding must be non-negative");
+
        memset(&nopts, 0, sizeof(nopts));
        nopts.indent = opts && opts->indent ? opts->indent : "";
        nopts.nl = opts && opts->nl ? opts->nl : "\n";

>  	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
>  	if (!column_active(colopts)) {
>  		display_plain(list, "", "\n");
> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
>  		strvec_pushf(argv, "--width=%d", opts->width);
>  	if (opts && opts->indent)
>  		strvec_pushf(argv, "--indent=%s", opts->indent);
> -	if (opts && opts->padding)
> +	if (opts && 0 <= opts->padding)

This also differs from Junio's changes.

>  		strvec_pushf(argv, "--padding=%d", opts->padding);
>  
>  	fflush(stdout);
> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> index 348cc406582..d5b98e615bc 100755
> --- a/t/t9002-column.sh
> +++ b/t/t9002-column.sh
> @@ -196,4 +196,15 @@ EOF
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'padding must be non-negative' '
> +	cat >input <<\EOF &&
> +1 2 3 4 5 6
> +EOF
> +	cat >expected <<\EOF &&
> +fatal: --padding must be non-negative
> +EOF
> +	test_must_fail git column --mode=column --padding=-1 <input >actual 2>&1 &&
> +	test_cmp expected actual
> +'
> +
>  test_done

OK

> -- 
> 2.43.0
> 

Re: [PATCH v2] column: disallow negative padding

[Rubén Justo]

On 11/2/24 23:47, Rubén Justo wrote:
> On 11-feb-2024 20:27:49, Kristoffer Haugsbakk wrote:
>> A negative padding does not make sense and can cause errors in the
>> memory allocator since it’s interpreted as an unsigned integer.
>>
>> Disallow negative padding. Also guard against negative padding in
>> `column.c` where it is conditionally used.
>>
>> Reported-by: Tiago Pascoal <tiago@pascoal.net>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>> ---
>>
>> Notes (series):
>>     v2:
>>     • Incorporate Junio’s changes (guard against negative padding in
>>       `column.c`)
>>     • Tweak commit message based on Junio’s analysis
>>     • Use gettext for error message
>>       • However I noticed that the “translation string” from `fast-import`
>>         isn’t a translation string. So let’s invent a new one and use a
>>         parameter so that it can be used elsewhere.
>>     • Make a test
>>
>>  builtin/column.c  |  2 ++
>>  column.c          |  4 ++--
>>  t/t9002-column.sh | 11 +++++++++++
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/column.c b/builtin/column.c
>> index e80218f81f9..10ff7e01668 100644
>> --- a/builtin/column.c
>> +++ b/builtin/column.c
>> @@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
>>  	memset(&copts, 0, sizeof(copts));
>>  	copts.padding = 1;
>>  	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
>> +	if (copts.padding < 0)
>> +		die(_("%s must be non-negative"), "--padding");
> 
> We clearly inform the user and die.  No more OOM errors, or worse.
> Good.
> 
> And the message avoids translation problems.  Excellent.
> 
>>  	if (argc)
>>  		usage_with_options(builtin_column_usage, options);
>>  	if (real_command || command) {
>> diff --git a/column.c b/column.c
>> index ff2f0abf399..c723428bc70 100644
>> --- a/column.c
>> +++ b/column.c
>> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,
>>  	memset(&nopts, 0, sizeof(nopts));
>>  	nopts.indent = opts && opts->indent ? opts->indent : "";
>>  	nopts.nl = opts && opts->nl ? opts->nl : "\n";
>> -	nopts.padding = opts ? opts->padding : 1;
>> +	nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;
> 
> This changes what Junio proposed.  Is this on purpose?
> 
> While we're here, I wonder if silently ignoring a negative value in
> .padding is the right thing to do.
> 
> There are several callers of print_columns():
> 
> builtin/branch.c:           print_columns(&output, colopts, NULL);
> builtin/clean.c:    print_columns(&list, colopts, &copts);
> builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);
> builtin/column.c:    print_columns(&list, colopts, &copts);
> help.c:     print_columns(&list, colopts, &copts);
> wt-status.c:       print_columns(&output, s->colopts, &copts);
> 
> I haven't checked it thoroughly but it seems we don't need to add the
> check we're adding to builtin/column.c, to any of the other callers.
> However, it is possible that these or other new callers may need it in
> the future.  If so, we should consider doing something like:
> 
> diff --git a/column.c b/column.c
> index c723428bc7..4f870c725f 100644
> --- a/column.c
> +++ b/column.c
> @@ -186,6 +186,9 @@ void print_columns(const struct string_list *list, unsigned int colopts,
>                 return;
>         assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
> 
> +       if (opts && (0 <= opts->padding))

Oops.  Of course, I mean:
+       if (opts && (0 > opts->padding))

Sorry.

> +               BUG("padding must be non-negative");
> +
>         memset(&nopts, 0, sizeof(nopts));
>         nopts.indent = opts && opts->indent ? opts->indent : "";
>         nopts.nl = opts && opts->nl ? opts->nl : "\n";
> 
>>  	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
>>  	if (!column_active(colopts)) {
>>  		display_plain(list, "", "\n");
>> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)
>>  		strvec_pushf(argv, "--width=%d", opts->width);
>>  	if (opts && opts->indent)
>>  		strvec_pushf(argv, "--indent=%s", opts->indent);
>> -	if (opts && opts->padding)
>> +	if (opts && 0 <= opts->padding)
> 
> This also differs from Junio's changes.
> 
>>  		strvec_pushf(argv, "--padding=%d", opts->padding);
>>  
>>  	fflush(stdout);
>> diff --git a/t/t9002-column.sh b/t/t9002-column.sh
>> index 348cc406582..d5b98e615bc 100755
>> --- a/t/t9002-column.sh
>> +++ b/t/t9002-column.sh
>> @@ -196,4 +196,15 @@ EOF
>>  	test_cmp expected actual
>>  '
>>  
>> +test_expect_success 'padding must be non-negative' '
>> +	cat >input <<\EOF &&
>> +1 2 3 4 5 6
>> +EOF
>> +	cat >expected <<\EOF &&
>> +fatal: --padding must be non-negative
>> +EOF
>> +	test_must_fail git column --mode=column --padding=-1 <input >actual 2>&1 &&
>> +	test_cmp expected actual
>> +'
>> +
>>  test_done
> 
> OK
> 
>> -- 
>> 2.43.0
>>

Re: [PATCH v2] column: disallow negative padding

[Kristoffer Haugsbakk]

Hey, thanks for the review

On Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:
>>  	if (argc)
>>  		usage_with_options(builtin_column_usage, options);
>>  	if (real_command || command) {
>> diff --git a/column.c b/column.c
>> index ff2f0abf399..c723428bc70 100644
>> --- a/column.c
>> +++ b/column.c
>> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,
>>  	memset(&nopts, 0, sizeof(nopts));
>>  	nopts.indent = opts && opts->indent ? opts->indent : "";
>>  	nopts.nl = opts && opts->nl ? opts->nl : "\n";
>> -	nopts.padding = opts ? opts->padding : 1;
>> +	nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;
>
> This changes what Junio proposed.  Is this on purpose?

Yes https://lore.kernel.org/git/3380df68-83fb-417b-a490-71614edc342f@app.fastmail.com/T/#m63ca728414def19b7a0c83ec76a8c1f2de68ffbb

-- 
Kristoffer Haugsbakk

Re: git column fails (or crashes) if padding is negative

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Fri, Feb 9, 2024, at 18:57, Junio C Hamano wrote:
>>  builtin/column.c | 2 ++
>>  column.c         | 4 ++--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> […]
>> diff --git c/column.c w/column.c
>> index ff2f0abf39..9cc703832a 100644
>> --- c/column.c
>> +++ w/column.c
>> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list,
>> unsigned int colopts,
>>  	memset(&nopts, 0, sizeof(nopts));
>>  	nopts.indent = opts && opts->indent ? opts->indent : "";
>>  	nopts.nl = opts && opts->nl ? opts->nl : "\n";
>> -	nopts.padding = opts ? opts->padding : 1;
>> +	nopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;
>
> If these two are meant to check the same condition as in
> `builtin/column.c`, shouldn’t it be `0 <= opts->padding`?

Good eyes.  Otherwise we lose the ability to set the padding to 0.

Re: [PATCH v2] column: disallow negative padding

[Kristoffer Haugsbakk]

On Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:
> While we're here, I wonder if silently ignoring a negative value in
> .padding is the right thing to do.
>
> There are several callers of print_columns():
>
> builtin/branch.c:           print_columns(&output, colopts, NULL);
> builtin/clean.c:    print_columns(&list, colopts, &copts);
> builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);
> builtin/column.c:    print_columns(&list, colopts, &copts);
> help.c:     print_columns(&list, colopts, &copts);
> wt-status.c:       print_columns(&output, s->colopts, &copts);
>
> I haven't checked it thoroughly but it seems we don't need to add the
> check we're adding to builtin/column.c, to any of the other callers.
> However, it is possible that these or other new callers may need it in
> the future.  If so, we should consider doing something like:
>
> diff --git a/column.c b/column.c
> index c723428bc7..4f870c725f 100644
> --- a/column.c
> +++ b/column.c
> @@ -186,6 +186,9 @@ void print_columns(const struct string_list *list,
> unsigned int colopts,
>                 return;
>         assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
>
> +       if (opts && (0 <= opts->padding))
> +               BUG("padding must be non-negative");
> +

Sure, I could add a `BUG` for `0 > opts->padding` in v3.

Re: [PATCH v2] column: disallow negative padding

[Rubén Justo]

On 12-feb-2024 17:50:54, Kristoffer Haugsbakk wrote:
> On Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:
> > While we're here, I wonder if silently ignoring a negative value in
> > .padding is the right thing to do.
> >
> > There are several callers of print_columns():
> >
> > builtin/branch.c:           print_columns(&output, colopts, NULL);
> > builtin/clean.c:    print_columns(&list, colopts, &copts);
> > builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);
> > builtin/column.c:    print_columns(&list, colopts, &copts);
> > help.c:     print_columns(&list, colopts, &copts);
> > wt-status.c:       print_columns(&output, s->colopts, &copts);
> >
> > I haven't checked it thoroughly but it seems we don't need to add the
> > check we're adding to builtin/column.c, to any of the other callers.
> > However, it is possible that these or other new callers may need it in
> > the future.  If so, we should consider doing something like:
> >
> > diff --git a/column.c b/column.c
> > index c723428bc7..4f870c725f 100644
> > --- a/column.c
> > +++ b/column.c
> > @@ -186,6 +186,9 @@ void print_columns(const struct string_list *list,
> > unsigned int colopts,
> >                 return;
> >         assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
> >
> > +       if (opts && (0 > opts->padding))

;-) (fixed)

> > +               BUG("padding must be non-negative");
> > +
> 
> Sure, I could add a `BUG` for `0 > opts->padding` in v3.

Thank you for considering it.

[PATCH v3 0/2] column: disallow negative padding

[Kristoffer Haugsbakk]

Fix bug in git-column(1): a user can pass a negative `padding` which
causes issues inside the memory allocator.

§ Changes in v3

Incorporate Ruben’s suggestion about guarding against negative padding
with `BUG` in `column.c` (not `builtin/column.c`). This then supersedes
Junio’s extra conditional checks since they are no longer needed. The
series gets split into two patches.

Cc: Tiago Pascoal <tiago@pascoal.net>
Cc: Chris Torek <chris.torek@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Rubén Justo <rjusto@gmail.com>

Kristoffer Haugsbakk (2):
  column: disallow negative padding
  column: guard against negative padding

 builtin/column.c  |  2 ++
 column.c          |  4 ++++
 t/t9002-column.sh | 11 +++++++++++
 3 files changed, 17 insertions(+)

Range-diff against v2:
1:  1c959378cf4 ! 1:  4cac42ca6f8 column: disallow negative padding
    @@ Commit message
         A negative padding does not make sense and can cause errors in the
         memory allocator since it’s interpreted as an unsigned integer.
     
    -    Disallow negative padding. Also guard against negative padding in
    -    `column.c` where it is conditionally used.
    -
         Reported-by: Tiago Pascoal <tiago@pascoal.net>
    -    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    -
    - ## Notes (series) ##
    -    v2:
    -    • Incorporate Junio’s changes (guard against negative padding in
    -      `column.c`)
    -    • Tweak commit message based on Junio’s analysis
    -    • Use gettext for error message
    -      • However I noticed that the “translation string” from `fast-import`
    -        isn’t a translation string. So let’s invent a new one and use a
    -        parameter so that it can be used elsewhere.
    -    • Make a test
    -
      ## builtin/column.c ##
     @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix)
      	memset(&copts, 0, sizeof(copts));
    @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix
      		usage_with_options(builtin_column_usage, options);
      	if (real_command || command) {
     
    - ## column.c ##
    -@@ column.c: void print_columns(const struct string_list *list, unsigned int colopts,
    - 	memset(&nopts, 0, sizeof(nopts));
    - 	nopts.indent = opts && opts->indent ? opts->indent : "";
    - 	nopts.nl = opts && opts->nl ? opts->nl : "\n";
    --	nopts.padding = opts ? opts->padding : 1;
    -+	nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;
    - 	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
    - 	if (!column_active(colopts)) {
    - 		display_plain(list, "", "\n");
    -@@ column.c: int run_column_filter(int colopts, const struct column_options *opts)
    - 		strvec_pushf(argv, "--width=%d", opts->width);
    - 	if (opts && opts->indent)
    - 		strvec_pushf(argv, "--indent=%s", opts->indent);
    --	if (opts && opts->padding)
    -+	if (opts && 0 <= opts->padding)
    - 		strvec_pushf(argv, "--padding=%d", opts->padding);
    - 
    - 	fflush(stdout);
    -
      ## t/t9002-column.sh ##
     @@ t/t9002-column.sh: EOF
      	test_cmp expected actual
-:  ----------- > 2:  9355fc98e3d column: guard against negative padding
-- 
2.43.0

[PATCH v3 1/2] column: disallow negative padding

[Kristoffer Haugsbakk]

A negative padding does not make sense and can cause errors in the
memory allocator since it’s interpreted as an unsigned integer.

Reported-by: Tiago Pascoal <tiago@pascoal.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/column.c  |  2 ++
 t/t9002-column.sh | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/builtin/column.c b/builtin/column.c
index e80218f81f9..10ff7e01668 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)
 	memset(&copts, 0, sizeof(copts));
 	copts.padding = 1;
 	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
+	if (copts.padding < 0)
+		die(_("%s must be non-negative"), "--padding");
 	if (argc)
 		usage_with_options(builtin_column_usage, options);
 	if (real_command || command) {
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 348cc406582..d5b98e615bc 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -196,4 +196,15 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'padding must be non-negative' '
+	cat >input <<\EOF &&
+1 2 3 4 5 6
+EOF
+	cat >expected <<\EOF &&
+fatal: --padding must be non-negative
+EOF
+	test_must_fail git column --mode=column --padding=-1 <input >actual 2>&1 &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.43.0

[PATCH v3 2/2] column: guard against negative padding

[Kristoffer Haugsbakk]

Make sure that client code can’t pass in a negative padding by accident.

Suggested-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    Apparently these are the only publicly-visible functions that use this
    struct according to `column.h`.

 column.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/column.c b/column.c
index ff2f0abf399..50bbccc92ee 100644
--- a/column.c
+++ b/column.c
@@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,
 {
 	struct column_options nopts;
 
+	if (opts && (0 > opts->padding))
+		BUG("padding must be non-negative");
 	if (!list->nr)
 		return;
 	assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
@@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)
 {
 	struct strvec *argv;
 
+	if (opts && (0 > opts->padding))
+		BUG("padding must be non-negative");
 	if (fd_out != -1)
 		return -1;
 
-- 
2.43.0

Re: [PATCH v3 2/2] column: guard against negative padding

[Junio C Hamano]

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

> Make sure that client code can’t pass in a negative padding by accident.
>
> Suggested-by: Rubén Justo <rjusto@gmail.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> Notes (series):
>     Apparently these are the only publicly-visible functions that use this
>     struct according to `column.h`.
>
>  column.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/column.c b/column.c
> index ff2f0abf399..50bbccc92ee 100644
> --- a/column.c
> +++ b/column.c
> @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,
>  {
>  	struct column_options nopts;
>  
> +	if (opts && (0 > opts->padding))
> +		BUG("padding must be non-negative");

The only two current callers may happen to be "git branch" that
passes NULL as opts, and "git clean" that passes 2 in opts->padding,
so this BUG() will not trigger.  Once we add new callers to this
function, or update the current callers, this safety start to matter.

The actual breakage from a negative padding happens in layout(),
so another option would be to have this guard there, which will
protect us from having new callers of that function as well, or
its caller display_table(), but these have only one caller each,
so having the guard print_columns() here, that is the closest to
the callers would be fine.

>  	if (!list->nr)
>  		return;
>  	assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
> @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)
>  {
>  	struct strvec *argv;
>  
> +	if (opts && (0 > opts->padding))
> +		BUG("padding must be non-negative");

This one happens to be safe currently because "git tag" passes 2 in
opts->padding, but I do not think this is needed.

We will pass these through to "git column" and the negative padding
will be caught as an error there anyway, no?  So whether "git tag"
is updated or a new caller of run_column_filter() is added, the
developer will already notice it (and they will have to protect
themselves just like the [1/2] of your series did for "git column"
itself).

>  	if (fd_out != -1)
>  		return -1;

Re: [PATCH v3 2/2] column: guard against negative padding

[Rubén Justo]

On 13-feb-2024 09:06:46, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> 
> > Make sure that client code can’t pass in a negative padding by accident.
> >
> > Suggested-by: Rubén Justo <rjusto@gmail.com>
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
> >
> > Notes (series):
> >     Apparently these are the only publicly-visible functions that use this
> >     struct according to `column.h`.
> >
> >  column.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/column.c b/column.c
> > index ff2f0abf399..50bbccc92ee 100644
> > --- a/column.c
> > +++ b/column.c
> > @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,
> >  {
> >  	struct column_options nopts;
> >  
> > +	if (opts && (0 > opts->padding))
> > +		BUG("padding must be non-negative");
> 
> The only two current callers may happen to be "git branch" that
> passes NULL as opts, and "git clean" that passes 2 in opts->padding,
> so this BUG() will not trigger.  Once we add new callers to this
> function, or update the current callers, this safety start to matter.
> 
> The actual breakage from a negative padding happens in layout(),
> so another option would be to have this guard there, which will
> protect us from having new callers of that function as well, or
> its caller display_table(), but these have only one caller each,
> so having the guard print_columns() here, that is the closest to
> the callers would be fine.
> 
> >  	if (!list->nr)
> >  		return;
> >  	assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
> > @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)
> >  {
> >  	struct strvec *argv;
> >  
> > +	if (opts && (0 > opts->padding))
> > +		BUG("padding must be non-negative");
> 
> This one happens to be safe currently because "git tag" passes 2 in
> opts->padding, but I do not think this is needed.

At first glance, I also thought this was not necessary.

However, callers of run_column_filter() might forget to check the return
value, and the BUG() triggered by the underlying process could be buried
and ignored.  Having the BUG() here, in the same process, makes it more
noticeable.

Based on this, I'm not opposed to this change.

Re: [PATCH v3 0/2] column: disallow negative padding

[Rubén Justo]

On 13-feb-2024 17:01:19, Kristoffer Haugsbakk wrote:

> The series gets split into two patches.

Very good.

> 
> Cc: Tiago Pascoal <tiago@pascoal.net>
> Cc: Chris Torek <chris.torek@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Rubén Justo <rjusto@gmail.com>
> 
> Kristoffer Haugsbakk (2):
>   column: disallow negative padding
>   column: guard against negative padding
> 
>  builtin/column.c  |  2 ++
>  column.c          |  4 ++++
>  t/t9002-column.sh | 11 +++++++++++
>  3 files changed, 17 insertions(+)
> 
> Range-diff against v2:
> 1:  1c959378cf4 ! 1:  4cac42ca6f8 column: disallow negative padding
>     @@ Commit message
>          A negative padding does not make sense and can cause errors in the
>          memory allocator since it’s interpreted as an unsigned integer.
>      
>     -    Disallow negative padding. Also guard against negative padding in
>     -    `column.c` where it is conditionally used.
>     -
>          Reported-by: Tiago Pascoal <tiago@pascoal.net>
>     -    Helped-by: Junio C Hamano <gitster@pobox.com>
>          Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>      
>     -
>     - ## Notes (series) ##
>     -    v2:
>     -    • Incorporate Junio’s changes (guard against negative padding in
>     -      `column.c`)
>     -    • Tweak commit message based on Junio’s analysis
>     -    • Use gettext for error message
>     -      • However I noticed that the “translation string” from `fast-import`
>     -        isn’t a translation string. So let’s invent a new one and use a
>     -        parameter so that it can be used elsewhere.
>     -    • Make a test
>     -
>       ## builtin/column.c ##
>      @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix)
>       	memset(&copts, 0, sizeof(copts));
>     @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix
>       		usage_with_options(builtin_column_usage, options);
>       	if (real_command || command) {
>      
>     - ## column.c ##
>     -@@ column.c: void print_columns(const struct string_list *list, unsigned int colopts,
>     - 	memset(&nopts, 0, sizeof(nopts));
>     - 	nopts.indent = opts && opts->indent ? opts->indent : "";
>     - 	nopts.nl = opts && opts->nl ? opts->nl : "\n";
>     --	nopts.padding = opts ? opts->padding : 1;
>     -+	nopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;
>     - 	nopts.width = opts && opts->width ? opts->width : term_columns() - 1;
>     - 	if (!column_active(colopts)) {
>     - 		display_plain(list, "", "\n");
>     -@@ column.c: int run_column_filter(int colopts, const struct column_options *opts)
>     - 		strvec_pushf(argv, "--width=%d", opts->width);
>     - 	if (opts && opts->indent)
>     - 		strvec_pushf(argv, "--indent=%s", opts->indent);
>     --	if (opts && opts->padding)
>     -+	if (opts && 0 <= opts->padding)
>     - 		strvec_pushf(argv, "--padding=%d", opts->padding);
>     - 
>     - 	fflush(stdout);
>     -
>       ## t/t9002-column.sh ##
>      @@ t/t9002-column.sh: EOF
>       	test_cmp expected actual
> -:  ----------- > 2:  9355fc98e3d column: guard against negative padding
> -- 
> 2.43.0
> 

The BUG() in run_column_filter() may be questionable, but overall this
v3 LGTM.

Thanks Kristoffer for your work.  And also thanks to Tiago for
reporting.


    * P.D. *
    
    Thinking about this in a more general way, I've found that this kind
    of error has hit us several times:
    
      - 953aa54e1a (pack-objects: clamp negative window size to 0, 2021-05-01)
      - 6d52b6a5df (pack-objects: clamp negative depth to 0, 2021-05-01)
    
    Maybe the source of this error is how easy is to forget that
    OPT_INTEGER can accept negative values (after all, that's what an
    integer is).
    
    There are not many users of OPT_INTEGER, and a quick check gives me
    the impression (maybe wrong...) that many of them do not expect
    negative values.
    
    Maybe we should consider having an OPT_INTEGER that fails if the
    value supplied is negative.  Ideally, some kind of opt-in machinery
    could be desirable, I think, for example to include/exclude:

    	- negative values
    	- "0"  ( may not be a desired value )
    	- "-1" ( may have some special meaning )
    	- ...
    
    I'll leave the idea here, just in case it inspires someone.  Thank
    you.

Re: [PATCH v3 2/2] column: guard against negative padding

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> This one happens to be safe currently because "git tag" passes 2 in
>> opts->padding, but I do not think this is needed.
>
> At first glance, I also thought this was not necessary.
>
> However, callers of run_column_filter() might forget to check the return
> value, and the BUG() triggered by the underlying process could be buried
> and ignored.  Having the BUG() here, in the same process, makes it more
> noticeable.

The point of BUG() is to help developers catch the silly breakage
before it excapes from the lab, and we can expect these careless
developers to ignore the return value.  But "column --padding=-1"
invoked as a subprocess will show a human-readable error message
to such a developer, so it is less important than the BUG() in the
other place.

There is no black or white decision, but this one is much less
darker gray than the other one is.

Re: [PATCH v3 2/2] column: guard against negative padding

[Rubén Justo]

On 13-feb-2024 11:39:11, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> This one happens to be safe currently because "git tag" passes 2 in
> >> opts->padding, but I do not think this is needed.
> >
> > At first glance, I also thought this was not necessary.
> >
> > However, callers of run_column_filter() might forget to check the return
> > value, and the BUG() triggered by the underlying process could be buried
> > and ignored.  Having the BUG() here, in the same process, makes it more
> > noticeable.
> 
> The point of BUG() is to help developers catch the silly breakage
> before it excapes from the lab, and we can expect these careless
> developers to ignore the return value.  But "column --padding=-1"
> invoked as a subprocess will show a human-readable error message
> to such a developer, so it is less important than the BUG() in the
> other place.
> 
> There is no black or white decision, but this one is much less
> darker gray than the other one is.

I've checked this, without that BUG(), and the result has not been
pretty:

diff --git a/builtin/tag.c b/builtin/tag.c
index 37473ac21f..e15dfa73d2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
                if (column_active(colopts)) {
                        struct column_options copts;
                        memset(&copts, 0, sizeof(copts));
-                       copts.padding = 2;
+                       copts.padding = -1;
                        run_column_filter(colopts, &copts);
                }
                filter.name_patterns = argv;

I can imagine a future change that opens that current "2" to the user.
And the possible report from a user who tries "-1" would not be easy.

But I agree with you, that BUG() does not leave a good taste in the
mouth.

Maybe we should refactor run_column_filter(), I don't know, but I think
that is outside of the scope of this series.

Re: [PATCH v3 0/2] column: disallow negative padding

[Kristoffer Haugsbakk]

On Tue, Feb 13, 2024, at 20:27, Rubén Justo wrote:
>     * P.D. *
>
>     Thinking about this in a more general way, I've found that this kind
>     of error has hit us several times:
>
>       - 953aa54e1a (pack-objects: clamp negative window size to 0, 2021-05-01)
>       - 6d52b6a5df (pack-objects: clamp negative depth to 0, 2021-05-01)
>
>     Maybe the source of this error is how easy is to forget that
>     OPT_INTEGER can accept negative values (after all, that's what an
>     integer is).
>
>     There are not many users of OPT_INTEGER, and a quick check gives me
>     the impression (maybe wrong...) that many of them do not expect
>     negative values.
>
>     Maybe we should consider having an OPT_INTEGER that fails if the
>     value supplied is negative.  Ideally, some kind of opt-in machinery
>     could be desirable, I think, for example to include/exclude:
>
>     	- negative values
>     	- "0"  ( may not be a desired value )
>     	- "-1" ( may have some special meaning )
>     	- ...
>
>     I'll leave the idea here, just in case it inspires someone.  Thank
>     you.

Thanks to both for providing a wider perspective on guarding against
such bugs.

And this is an excellent point. I don’t know anything about the opt-args
implementation but it would be great to guard against user-supplied
values through the option parsing library.

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH v3 2/2] column: guard against negative padding

[Kristoffer Haugsbakk]

On Tue, Feb 13, 2024, at 20:56, Rubén Justo wrote:
> On 13-feb-2024 11:39:11, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> The point of BUG() is to help developers catch the silly breakage
>> before it excapes from the lab, and we can expect these careless
>> developers to ignore the return value.  But "column --padding=-1"
>> invoked as a subprocess will show a human-readable error message
>> to such a developer, so it is less important than the BUG() in the
>> other place.
>>
>> There is no black or white decision, but this one is much less
>> darker gray than the other one is.
>
> I've checked this, without that BUG(), and the result has not been
> pretty:
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 37473ac21f..e15dfa73d2 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>                 if (column_active(colopts)) {
>                         struct column_options copts;
>                         memset(&copts, 0, sizeof(copts));
> -                       copts.padding = 2;
> +                       copts.padding = -1;
>                         run_column_filter(colopts, &copts);
>                 }
>                 filter.name_patterns = argv;
>
> I can imagine a future change that opens that current "2" to the user.
> And the possible report from a user who tries "-1" would not be easy.
>
> But I agree with you, that BUG() does not leave a good taste in the
> mouth.
>
> Maybe we should refactor run_column_filter(), I don't know, but I think
> that is outside of the scope of this series.

Thanks for trying that out—some very topical testing!

I will take the night to think about v4. But I will defer to the
reviewers’ judgement on the scope of this series/change.

(All I know is that it can be tricky balancing such defensive checks
with readability and maintanability.)

Thanks

-- 
Kristoffer Haugsbakk

Re: [PATCH v3 0/2] column: disallow negative padding

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

>>     There are not many users of OPT_INTEGER, and a quick check gives me
>>     the impression (maybe wrong...) that many of them do not expect
>>     negative values.
>>
>>     Maybe we should consider having an OPT_INTEGER that fails if the
>>     value supplied is negative.  Ideally, some kind of opt-in machinery
>>     could be desirable, I think, for example to include/exclude:
>>
>>     	- negative values
>>     	- "0"  ( may not be a desired value )
>>     	- "-1" ( may have some special meaning )
>>     	- ...
>>
>>     I'll leave the idea here, just in case it inspires someone.  Thank
>>     you.

Interesting.

I wonder if there is a correlation between "never negative" and
"handy if it took scale unit (like 2k to mean 2048)"?  If so,
perhaps we can replace those that use OPT_INTEGER to use
OPT_MAGNITUDE instead.

Thanks.

Re: [PATCH v3 2/2] column: guard against negative padding

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> (All I know is that it can be tricky balancing such defensive checks
> with readability and maintanability.)

Personally I think v3 is a good enough place to stop and we are now
entering into the realm of diminishing returns.

Thanks.

Re: [PATCH v3 2/2] column: guard against negative padding

[Rubén Justo]

On 13-feb-2024 11:39:11, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> This one happens to be safe currently because "git tag" passes 2 in
> >> opts->padding, but I do not think this is needed.
> >
> > At first glance, I also thought this was not necessary.
> >
> > However, callers of run_column_filter() might forget to check the return
> > value, and the BUG() triggered by the underlying process could be buried
> > and ignored.  Having the BUG() here, in the same process, makes it more
> > noticeable.
> 
> The point of BUG() is to help developers catch the silly breakage
> before it excapes from the lab, and we can expect these careless
> developers to ignore the return value.  But "column --padding=-1"
> invoked as a subprocess will show a human-readable error message
> to such a developer, so it is less important than the BUG() in the
> other place.

Thinking again about this; you are right.  That BUG() in
run_column_filter() does not make sense in this series.

It is addressing a different error, and perhaps a solution could be:

--- >8 ---
Subject: [PATCH] tag: error when git-column fails

If the user asks for the list of tags to be displayed in columns
("--columns"), a child git-column process is used to format the output
as expected.

In a rare situation where we encounter a problem spawning that child
process, we will work erroneously.

Make noticeable we're having a problem executing git-column, so the user
can act accordingly.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/tag.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 37473ac21f..30532b76d5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			struct column_options copts;
 			memset(&copts, 0, sizeof(copts));
 			copts.padding = 2;
-			run_column_filter(colopts, &copts);
+			if (run_column_filter(colopts, &copts))
+				die(_("could not start 'git column'")
 		}
 		filter.name_patterns = argv;
 		ret = list_tags(&filter, sorting, &format);
-- 
2.43.0

[PATCH] tag: error when git-column fails

[Rubén Justo]

If the user asks for the list of tags to be displayed in columns
("--columns"), a child git-column process is used to format the output
as expected.

In a rare situation where we encounter a problem spawning that child
process, we will work erroneously.

Make noticeable we're having a problem executing git-column, so the user
can act accordingly.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/tag.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 37473ac21f..19a7e06bf4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			struct column_options copts;
 			memset(&copts, 0, sizeof(copts));
 			copts.padding = 2;
-			run_column_filter(colopts, &copts);
+			if (run_column_filter(colopts, &copts))
+				die(_("could not start 'git column'"));
 		}
 		filter.name_patterns = argv;
 		ret = list_tags(&filter, sorting, &format);
-- 
2.43.0

Re: [PATCH] tag: error when git-column fails

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> @@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  			struct column_options copts;
>  			memset(&copts, 0, sizeof(copts));
>  			copts.padding = 2;
> -			run_column_filter(colopts, &copts);
> +			if (run_column_filter(colopts, &copts))
> +				die(_("could not start 'git column'"));

Nice.  This obvious omission should have been here from the day one.

Will queue.  Thanks.

>  		}
>  		filter.name_patterns = argv;
>  		ret = list_tags(&filter, sorting, &format);