Re: [PATCH v2] column: disallow negative padding

["Rubén Justo" <rjusto@gmail.com>]

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