Re: git column fails (or crashes) if padding is negative
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 9, 2024, 17:57 UTC
- Message-ID
- <xmqqttmhfrko.fsf@gitster.g>
- In-Reply-To
- <571fb353-af1d-4cc9-a2c2-197296685623@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
Show 12 quoted lines
> ``` > $ seq 1 24 | git column --mode=column --padding=-1 > 12345678910<binary?><numbers> > $ seq 1 24 | git column --mode=column --padding=-3 > fatal: Data too large to fit into virtual memory space. > $ seq 1 24 | git column --mode=column --padding=-5 > fatal: Out of memory, malloc failed (tried to allocate 18446744073709551614 bytes) > ``` > > This is an “Internal helper command” under the “plumbing” suite. And I > get the impression that sometimes these fallthroughs are treated as > “don’t do that”. But I don’t know.
If the nonsense input is easy to tell, then telling "don't feed nonsense input" to the user while rejecting such nonsense input would be a good idea.
> On the other hand it failing inside malloc looks weird. Why not catch > this before the malloc call is made?
Presumably, the parameter we prepare before calling malloc() is of unsigned type, and feeding a negative value to such a callchain would cast it to a large unsigned value?
Indeed, whereever cops.padding is referenced in column.c, it clearly is assumed that it is a non-negative value. *width accumulates the width of data items plus padding, and it also is used to divide some number to arrive at the number of columns, so by tweaking the padding to the right value, you probably should be able to cause division by zero, too, in column.c:layout().
Hopefully the attached would be a good place to start (I am not going to finish it with log message, tests, and fixes to other places).
builtin/column.c | 2 ++ column.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git c/builtin/column.c w/builtin/column.c index e80218f81f..8537d09d2b 100644 --- c/builtin/column.c +++ w/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(_("--padding must be non-negative")); if (argc) usage_with_options(builtin_column_usage, options); if (real_command || command) { diff --git c/column.c w/column.c index ff2f0abf39..9cc703832a 100644 --- c/column.c +++ w/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);