From: lavendarlatte <hyunjidev@xxxxxxxxx>
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 <hyunjidev@xxxxxxxxx>
---
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(+)
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
base-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff
--
gitgitgadget
