Re: [PATCH v2] column: disallow negative padding
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Feb 11, 2024, 22:47 UTC
- Message-ID
- <89d32a5f-b5ab-4773-bd9f-d33b4e348e15@gmail.com>
- In-Reply-To
- <1c959378cf495d7a3d70d0c7bdf08cc501ed6e5d.1707679627.git.code@khaugsbakk.name>
On 11-feb-2024 20:27:49, Kristoffer Haugsbakk wrote:
Show 37 quoted lines
> 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.
Show 13 quoted lines
> 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 >