Re: [PATCH v3 2/2] column: guard against negative padding

On 13-feb-2024 09:06:46, Junio C Hamano wrote:

Kristoffer Haugsbakk [off-list ref] writes:

quoted

Make sure that client code can’t pass in a negative padding by accident.

Suggested-by: Rubén Justo <redacted>
Signed-off-by: Kristoffer Haugsbakk <redacted>
---

Notes (series):
    Apparently these are the only publicly-visible functions that use this
    struct according to `column.h`.

 column.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/column.c b/column.c
index ff2f0abf399..50bbccc92ee 100644
--- a/column.c
+++ b/column.c

@@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,
 {
 	struct column_options nopts;
 
+	if (opts && (0 > opts->padding))
+		BUG("padding must be non-negative");

The only two current callers may happen to be "git branch" that
passes NULL as opts, and "git clean" that passes 2 in opts->padding,
so this BUG() will not trigger.  Once we add new callers to this
function, or update the current callers, this safety start to matter.

The actual breakage from a negative padding happens in layout(),
so another option would be to have this guard there, which will
protect us from having new callers of that function as well, or
its caller display_table(), but these have only one caller each,
so having the guard print_columns() here, that is the closest to
the callers would be fine.

quoted

 	if (!list->nr)
 		return;
 	assert((colopts & COL_ENABLE_MASK) != COL_AUTO);

@@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)
 {
 	struct strvec *argv;
 
+	if (opts && (0 > opts->padding))
+		BUG("padding must be non-negative");

This one happens to be safe currently because "git tag" passes 2 in
opts->padding, but I do not think this is needed.

At first glance, I also thought this was not necessary.

However, callers of run_column_filter() might forget to check the return
value, and the BUG() triggered by the underlying process could be buried
and ignored.  Having the BUG() here, in the same process, makes it more
noticeable.

Based on this, I'm not opposed to this change.