column: exit early when indent length is larger than width

Commit Message

lavendarlatte April 2, 2025, 1:45 a.m. UTC

From: lavendarlatte <[e-mail]>

The code exits with "fatal size_t overflow" when indent length is
larger than width. This is because when calculating cols of struct
column_data, unsigned underflow happens and cols is set to negative
value, then converted to size_t when calling REALLOC_ARRAY in
shrink_columns() function. This can lead to allocating extremely
large chunk of memory when succeeds, or crash when fails.

The change exits code early with failure reason to avoid underflow
and clarify argument limitations. This change ensures that cols is
always positive, making the code clearer. It also eliminates the need
for warning suppression related to signed-unsigned comparisons, as
cols can be safely converted to size_t.

Signed-off-by: Hyunji Choi <[e-mail]>
---
    column: exit early when indent length is larger than width
    
    I have sent this to git-security first for potential security check.
    Thank you Patrick for review and reply. Based on your comment I have
    updated BUG() to die() and added two tests. Also confirmed all existing
    column tests pass with new check.
    
    This is copy of his reply:
    
    Hi, thanks for your report!
    
    The use of both print_columns() and run_column_filter() is rather
    limited across the Git codebase and only covers across a small set of
    builtin commands:
    
     * git-branch(1), where the value can be changed via the --column
       command line option.
     * git-clean(1), where the values are hardcoded.
     * git-column(1), obviously.
     * git-help(1), but again the values are hardcoded.
     * git-status(1) via wt_longstatus_print_other(), but the values are
       hardcoded.
     * git-tag(1) with the --column command line option.
    
    So only git-branch(1), git-tag(1) and git-column(1) are relevant in this
    context, and I cannot think of any way to exploit these in a meaningful
    way. I also think that --column options are unlikely to be used in any
    scripts.
    
    So all in all I think it's fine to discuss this on our normal mailing
    list and fix the issue in the open. But I'd maybe wait a day or two for
    others to chime in before doing so. Given that these values are
    user-controlled we probably shouldn't use BUG() because it isn't. die()
    would likely be a better fit. We should also have one or two tests to
    verify that things work as expected.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1937%2Flavendarlatte%2Findent-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1937/lavendarlatte/indent-v1
Pull-Request: https://github.com/git/git/pull/1937

 column.c          |  6 ++++++
 t/t9002-column.sh | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)


base-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff

Patch

diff --git a/column.c b/column.c
index 93fae316b45..692fefafabd 100644
--- a/column.c
+++ b/column.c
@@ -186,6 +186,9 @@  void print_columns(const struct string_list *list, unsigned int colopts,
 
 	if (opts && (0 > opts->padding))
 		BUG("padding must be non-negative");
+	if (opts && (opts->width > 0) && opts->indent &&
+		(opts->width < strlen(opts->indent)))
+		die("length of indent cannot exceed width");
 	if (!list->nr)
 		return;
 	assert((colopts & COL_ENABLE_MASK) != COL_AUTO);
@@ -367,6 +370,9 @@  int run_column_filter(int colopts, const struct column_options *opts)
 
 	if (opts && (0 > opts->padding))
 		BUG("padding must be non-negative");
+	if (opts && (opts->width > 0) && opts->indent &&
+		(opts->width < strlen(opts->indent)))
+		die("length of indent cannot exceed width");
 	if (fd_out != -1)
 		return -1;
 
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 7353815c11b..1b448be4567 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -206,4 +206,26 @@  EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'length of indent cannot exceed width' '
+	cat >input <<\EOF &&
+1 2 3 4 5 6
+EOF
+	cat >expected <<\EOF &&
+fatal: length of indent cannot exceed width
+EOF
+	test_must_fail git column --mode=column --width=1 --indent="--" <input >actual 2>&1 &&
+	test_cmp expected actual
+'
+
+test_expect_success 'padding must be non-negative checked before indent length' '
+	cat >input <<\EOF &&
+1 2 3 4 5 6
+EOF
+	cat >expected <<\EOF &&
+fatal: --padding must be non-negative
+EOF
+	test_must_fail git column --mode=column --padding=-1 --width=1 --indent="--" <input >actual 2>&1 &&
+	test_cmp expected actual
+'
+
 test_done