| Message ID | cover.1707839454.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| Headers | show |
| Series | column: disallow negative padding | expand |
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.
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" <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.
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