| Message ID | 1c959378cf495d7a3d70d0c7bdf08cc501ed6e5d.1707679627.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] column: disallow negative padding | expand |
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 >
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 >>
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
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.
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.
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
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(-)