{"thread":{"id":"63237","subject":"[PATCH] column: exit early when indent length is larger than width","startedAt":"2025-04-02T01:45:18Z","lastAt":"2025-04-02T01:45:18Z","messageCount":1,"participants":["HJ C. via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"515475","messageId":"pull.1937.git.git.1743558315633.gitgitgadget@gmail.com","threadId":"63237","inReplyTo":null,"subject":"[PATCH] column: exit early when indent length is larger than width","fromName":"HJ C. via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-04-02T01:45:15Z","receivedAt":"2025-04-02T01:45:18Z","isPatch":true,"sender":{"key":"name:HJ C.","avatar":null},"body":"From: lavendarlatte <hyunjidev@gmail.com>\n\nThe code exits with \"fatal size_t overflow\" when indent length is\nlarger than width. This is because when calculating cols of struct\ncolumn_data, unsigned underflow happens and cols is set to negative\nvalue, then converted to size_t when calling REALLOC_ARRAY in\nshrink_columns() function. This can lead to allocating extremely\nlarge chunk of memory when succeeds, or crash when fails.\n\nThe change exits code early with failure reason to avoid underflow\nand clarify argument limitations. This change ensures that cols is\nalways positive, making the code clearer. It also eliminates the need\nfor warning suppression related to signed-unsigned comparisons, as\ncols can be safely converted to size_t.\n\nSigned-off-by: Hyunji Choi <hyunjidev@gmail.com>\n---\n    column: exit early when indent length is larger than width\n    \n    I have sent this to git-security first for potential security check.\n    Thank you Patrick for review and reply. Based on your comment I have\n    updated BUG() to die() and added two tests. Also confirmed all existing\n    column tests pass with new check.\n    \n    This is copy of his reply:\n    \n    Hi, thanks for your report!\n    \n    The use of both print_columns() and run_column_filter() is rather\n    limited across the Git codebase and only covers across a small set of\n    builtin commands:\n    \n     * git-branch(1), where the value can be changed via the --column\n       command line option.\n     * git-clean(1), where the values are hardcoded.\n     * git-column(1), obviously.\n     * git-help(1), but again the values are hardcoded.\n     * git-status(1) via wt_longstatus_print_other(), but the values are\n       hardcoded.\n     * git-tag(1) with the --column command line option.\n    \n    So only git-branch(1), git-tag(1) and git-column(1) are relevant in this\n    context, and I cannot think of any way to exploit these in a meaningful\n    way. I also think that --column options are unlikely to be used in any\n    scripts.\n    \n    So all in all I think it's fine to discuss this on our normal mailing\n    list and fix the issue in the open. But I'd maybe wait a day or two for\n    others to chime in before doing so. Given that these values are\n    user-controlled we probably shouldn't use BUG() because it isn't. die()\n    would likely be a better fit. We should also have one or two tests to\n    verify that things work as expected.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1937%2Flavendarlatte%2Findent-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1937/lavendarlatte/indent-v1\nPull-Request: https://github.com/git/git/pull/1937\n\n column.c          |  6 ++++++\n t/t9002-column.sh | 22 ++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/column.c b/column.c\nindex 93fae316b45..692fefafabd 100644\n--- a/column.c\n+++ b/column.c\n@@ -186,6 +186,9 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n \n \tif (opts && (0 > opts->padding))\n \t\tBUG(\"padding must be non-negative\");\n+\tif (opts && (opts->width > 0) && opts->indent &&\n+\t\t(opts->width < strlen(opts->indent)))\n+\t\tdie(\"length of indent cannot exceed width\");\n \tif (!list->nr)\n \t\treturn;\n \tassert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n@@ -367,6 +370,9 @@ int run_column_filter(int colopts, const struct column_options *opts)\n \n \tif (opts && (0 > opts->padding))\n \t\tBUG(\"padding must be non-negative\");\n+\tif (opts && (opts->width > 0) && opts->indent &&\n+\t\t(opts->width < strlen(opts->indent)))\n+\t\tdie(\"length of indent cannot exceed width\");\n \tif (fd_out != -1)\n \t\treturn -1;\n \ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 7353815c11b..1b448be4567 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -206,4 +206,26 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'length of indent cannot exceed width' '\n+\tcat >input <<\\EOF &&\n+1 2 3 4 5 6\n+EOF\n+\tcat >expected <<\\EOF &&\n+fatal: length of indent cannot exceed width\n+EOF\n+\ttest_must_fail git column --mode=column --width=1 --indent=\"--\" <input >actual 2>&1 &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'padding must be non-negative checked before indent length' '\n+\tcat >input <<\\EOF &&\n+1 2 3 4 5 6\n+EOF\n+\tcat >expected <<\\EOF &&\n+fatal: --padding must be non-negative\n+EOF\n+\ttest_must_fail git column --mode=column --padding=-1 --width=1 --indent=\"--\" <input >actual 2>&1 &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n\nbase-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff\n-- \ngitgitgadget\n"}]}