On 13-feb-2024 17:01:19, Kristoffer Haugsbakk wrote:
> The series gets split into two patches.
Very good.
>
> Cc: Tiago Pascoal <tiago@xxxxxxxxxxx>
> Cc: Chris Torek <chris.torek@xxxxxxxxx>
> Cc: Junio C Hamano <gitster@xxxxxxxxx>
> Cc: Rubén Justo <rjusto@xxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> - Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
> Signed-off-by: Kristoffer Haugsbakk <code@xxxxxxxxxxxxxxx>
>
> -
> - ## 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.
