Re: git column fails (or crashes) if padding is negative

[Junio C Hamano <gitster@pobox.com>]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> ```
> $ 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);