On 11-feb-2024 20:27:49, Kristoffer Haugsbakk wrote:

quoted

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 <redacted>
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Kristoffer Haugsbakk <redacted>
---

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.

quoted

 	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))