{"thread":{"id":"60883","subject":"git column fails (or crashes) if padding is negative","startedAt":"2024-02-09T14:21:27Z","lastAt":"2024-02-14T01:35:39Z","messageCount":32,"participants":["Tiago Pascoal","Kristoffer Haugsbakk","Junio C Hamano","Chris Torek","Rubén Justo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"488292","messageId":"AS8P189MB21977ACC4866D9836DA29082BC4B2@AS8P189MB2197.EURP189.PROD.OUTLOOK.COM","threadId":"60883","inReplyTo":null,"subject":"git column fails (or crashes) if padding is negative","fromName":"Tiago Pascoal","fromEmail":"tiago@pascoal.net","sentAt":"2024-02-09T14:21:25Z","receivedAt":"2024-02-09T14:21:27Z","isPatch":false,"sender":{"key":"tiago@pascoal.net","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nCall git column with a negative padding value, e.g. `git column --padding -1`\n\nIf the number if bigger than -3, the command will fail with the following error message:\n\n  Floating point exception\n\nIf the number is -5 or greater, the command will fail with the following error message:\n\n  fatal: Out of memory, malloc failed (tried to allocate 18446744073709551615 bytes)\n\neg:\n$ seq 1 100 | git column --mode=column  --padding=-5\nfatal: Out of memory, malloc failed (tried to allocate 18446744073709551615 bytes)\n\nWhat did you expect to happen? (Expected behavior)\n\nAn error message indicating that the padding value is invalid and not a crash.\n\nWhat happened instead? (Actual behavior)\n\nFailed command or even an OOM error depending on the padding value.\n\nWhat's different between what you expected and what actually happened?\n\nAnything else you want to add:\n\n\n[System Info]\ngit version:\ngit version 2.34.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64\ncompiler info: gnuc: 11.4\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show"},{"id":"488299","messageId":"571fb353-af1d-4cc9-a2c2-197296685623@app.fastmail.com","threadId":"60883","inReplyTo":"AS8P189MB21977ACC4866D9836DA29082BC4B2@AS8P189MB2197.EURP189.PROD.OUTLOOK.COM","subject":"Re: git column fails (or crashes) if padding is negative","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-09T16:27:59Z","receivedAt":"2024-02-09T16:28:33Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi\n\nI wasn’t able to reproduce quite what you got but kind of the same.\n\n```\n$ seq 1 24 | git column --mode=column --padding=-1\n12345678910<binary?><numbers>\n$ seq 1 24 | git column --mode=column --padding=-3\nfatal: Data too large to fit into virtual memory space.\n$ seq 1 24 | git column --mode=column --padding=-5\nfatal: Out of memory, malloc failed (tried to allocate 18446744073709551614 bytes)\n```\n\nThis is an “Internal helper command” under the “plumbing” suite. And I\nget the impression that sometimes these fallthroughs are treated as\n“don’t do that”. But I don’t know.\n\nOn the other hand it failing inside malloc looks weird. Why not catch\nthis before the malloc call is made?\n\n[System Info]\ngit version:\ngit version 2.43.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.5.0-17-generic #17~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Jan 16 14:32:32 UTC 2 x86_64\ncompiler info: gnuc: 11.4\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\n\n-- \nKristoffer Haugsbakk\n"},{"id":"488309","messageId":"76688ed2cc20031d70823d9f5d214f42b3bd1409.1707501064.git.code@khaugsbakk.name","threadId":"60883","inReplyTo":"AS8P189MB21977ACC4866D9836DA29082BC4B2@AS8P189MB2197.EURP189.PROD.OUTLOOK.COM","subject":"[PATCH] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-09T17:52:03Z","receivedAt":"2024-02-09T17:53:42Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"A negative padding can cause some problems in the memory allocator:\n\n• floating point exception\n• data too large to fit into virtual memory space\n• OOM\n\nDisallow negative padding. Reuse a translation string from\n`fast-import`.\n\nReported-by: Tiago Pascoal <tiago@pascoal.net>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n builtin/column.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/column.c b/builtin/column.c\nindex e80218f81f9..82902d149c2 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \tmemset(&copts, 0, sizeof(copts));\n \tcopts.padding = 1;\n \targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n+\tif (copts.padding < 0)\n+\t\tdie(\"%s: argument must be a non-negative integer\", \"padding\");\n \tif (argc)\n \t\tusage_with_options(builtin_column_usage, options);\n \tif (real_command || command) {\n-- \n2.43.0\n\n"},{"id":"488310","messageId":"xmqqttmhfrko.fsf@gitster.g","threadId":"60883","inReplyTo":"571fb353-af1d-4cc9-a2c2-197296685623@app.fastmail.com","subject":"Re: git column fails (or crashes) if padding is negative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-09T17:57:59Z","receivedAt":"2024-02-09T17:58:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n\n> ```\n> $ seq 1 24 | git column --mode=column --padding=-1\n> 12345678910<binary?><numbers>\n> $ seq 1 24 | git column --mode=column --padding=-3\n> fatal: Data too large to fit into virtual memory space.\n> $ seq 1 24 | git column --mode=column --padding=-5\n> fatal: Out of memory, malloc failed (tried to allocate 18446744073709551614 bytes)\n> ```\n>\n> This is an “Internal helper command” under the “plumbing” suite. And I\n> get the impression that sometimes these fallthroughs are treated as\n> “don’t do that”. But I don’t know.\n\nIf the nonsense input is easy to tell, then telling \"don't feed\nnonsense input\" to the user while rejecting such nonsense input\nwould be a good idea.\n\n> On the other hand it failing inside malloc looks weird. Why not catch\n> this before the malloc call is made?\n\nPresumably, the parameter we prepare before calling malloc() is of\nunsigned type, and feeding a negative value to such a callchain\nwould cast it to a large unsigned value?\n\nIndeed, whereever cops.padding is referenced in column.c, it clearly\nis assumed that it is a non-negative value.  *width accumulates the\nwidth of data items plus padding, and it also is used to divide some\nnumber to arrive at the number of columns, so by tweaking the padding\nto the right value, you probably should be able to cause division by\nzero, too, in column.c:layout().\n\nHopefully the attached would be a good place to start (I am not\ngoing to finish it with log message, tests, and fixes to other\nplaces).\n\n builtin/column.c | 2 ++\n column.c         | 4 ++--\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin/column.c w/builtin/column.c\nindex e80218f81f..8537d09d2b 100644\n--- c/builtin/column.c\n+++ w/builtin/column.c\n@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \tmemset(&copts, 0, sizeof(copts));\n \tcopts.padding = 1;\n \targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n+\tif (copts.padding < 0)\n+\t\tdie(_(\"--padding must be non-negative\"));\n \tif (argc)\n \t\tusage_with_options(builtin_column_usage, options);\n \tif (real_command || command) {\ndiff --git c/column.c w/column.c\nindex ff2f0abf39..9cc703832a 100644\n--- c/column.c\n+++ w/column.c\n@@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n \tmemset(&nopts, 0, sizeof(nopts));\n \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n-\tnopts.padding = opts ? opts->padding : 1;\n+\tnopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;\n \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n \tif (!column_active(colopts)) {\n \t\tdisplay_plain(list, \"\", \"\\n\");\n@@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\n \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n \tif (opts && opts->indent)\n \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n-\tif (opts && opts->padding)\n+\tif (opts && 0 < opts->padding)\n \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n \n \tfflush(stdout);\n"},{"id":"488312","messageId":"19119aa6-9a8c-44c6-af79-0ea6a8bcb181@app.fastmail.com","threadId":"60883","inReplyTo":"76688ed2cc20031d70823d9f5d214f42b3bd1409.1707501064.git.code@khaugsbakk.name","subject":"Re: [PATCH] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-09T18:26:54Z","receivedAt":"2024-02-09T18:27:16Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"I forgot tests.\n\n-- \nKristoffer\n"},{"id":"488347","messageId":"CAPx1GvdDvmBmvoktd7onB4mSzikKf4eWVWnrzrn8c8Y1RcRgsA@mail.gmail.com","threadId":"60883","inReplyTo":"19119aa6-9a8c-44c6-af79-0ea6a8bcb181@app.fastmail.com","subject":"Re: [PATCH] column: disallow negative padding","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-02-10T09:48:08Z","receivedAt":"2024-02-10T09:48:21Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk\n<code@khaugsbakk.name> wrote:\n> I forgot tests.\n\nYou presumably also wanted the `_` here for gettext-ing:\n\n> +               die(\"%s: argument must be a non-negative integer\", \"padding\");\n\nChris\n"},{"id":"488401","messageId":"3380df68-83fb-417b-a490-71614edc342f@app.fastmail.com","threadId":"60883","inReplyTo":"xmqqttmhfrko.fsf@gitster.g","subject":"Re: git column fails (or crashes) if padding is negative","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-11T17:08:10Z","receivedAt":"2024-02-11T17:08:31Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Feb 9, 2024, at 18:57, Junio C Hamano wrote:\n>  builtin/column.c | 2 ++\n>  column.c         | 4 ++--\n>  2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> […]\n> diff --git c/column.c w/column.c\n> index ff2f0abf39..9cc703832a 100644\n> --- c/column.c\n> +++ w/column.c\n> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list,\n> unsigned int colopts,\n>  \tmemset(&nopts, 0, sizeof(nopts));\n>  \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n>  \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n> -\tnopts.padding = opts ? opts->padding : 1;\n> +\tnopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;\n\nIf these two are meant to check the same condition as in\n`builtin/column.c`, shouldn’t it be `0 <= opts->padding`?\n\n>  \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n>  \tif (!column_active(colopts)) {\n>  \t\tdisplay_plain(list, \"\", \"\\n\");\n> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct\n> column_options *opts)\n>  \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n>  \tif (opts && opts->indent)\n>  \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n> -\tif (opts && opts->padding)\n> +\tif (opts && 0 < opts->padding)\n>  \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n>\n>  \tfflush(stdout);\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"488402","messageId":"9c00311d-e31c-428b-9c66-fef7ac8bfc76@app.fastmail.com","threadId":"60883","inReplyTo":"CAPx1GvdDvmBmvoktd7onB4mSzikKf4eWVWnrzrn8c8Y1RcRgsA@mail.gmail.com","subject":"Re: [PATCH] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-11T17:10:26Z","receivedAt":"2024-02-11T17:10:49Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sat, Feb 10, 2024, at 10:48, Chris Torek wrote:\n> On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk\n> <code@khaugsbakk.name> wrote:\n>> I forgot tests.\n>\n> You presumably also wanted the `_` here for gettext-ing:\n>\n>> +               die(\"%s: argument must be a non-negative integer\", \"padding\");\n>\n> Chris\n\nYeah, thanks. You probably saved me a v3. :)\n\nAlthough I failed to notice that the string I stole was just a plain\nstring, not a translation string. And apparently there are no generic\n“non-negative” translation strings. So I’ll just make a new one.\n\nCheers\n\n-- \nKristoffer Haugsbakk\n"},{"id":"488404","messageId":"xmqqle7q997e.fsf@gitster.g","threadId":"60883","inReplyTo":"9c00311d-e31c-428b-9c66-fef7ac8bfc76@app.fastmail.com","subject":"Re: [PATCH] column: disallow negative padding","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-11T17:55:49Z","receivedAt":"2024-02-11T17:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n\n> On Sat, Feb 10, 2024, at 10:48, Chris Torek wrote:\n>> On Sat, Feb 10, 2024 at 1:46 AM Kristoffer Haugsbakk\n>> <code@khaugsbakk.name> wrote:\n>>> I forgot tests.\n>>\n>> You presumably also wanted the `_` here for gettext-ing:\n>>\n>>> +               die(\"%s: argument must be a non-negative integer\", \"padding\");\n>>\n>> Chris\n>\n> Yeah, thanks. You probably saved me a v3. :)\n>\n> Although I failed to notice that the string I stole was just a plain\n> string, not a translation string. And apparently there are no generic\n> “non-negative” translation strings. So I’ll just make a new one.\n\nThe last time I took a look, I thought there were more than just the\nsingle entry point you patched that can feed negative padding into\nthe machinery?  Don't you need to cover them as well?\n\nThanks.\n"},{"id":"488408","messageId":"34c18fd0-1fa3-4226-b5a1-b019fcb05548@app.fastmail.com","threadId":"60883","inReplyTo":"xmqqle7q997e.fsf@gitster.g","subject":"Re: [PATCH] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-11T18:18:41Z","receivedAt":"2024-02-11T18:19:02Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sun, Feb 11, 2024, at 18:55, Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n>> Yeah, thanks. You probably saved me a v3. :)\n>>\n>> Although I failed to notice that the string I stole was just a plain\n>> string, not a translation string. And apparently there are no generic\n>> “non-negative” translation strings. So I’ll just make a new one.\n>\n> The last time I took a look, I thought there were more than just the\n> single entry point you patched that can feed negative padding into\n> the machinery?  Don't you need to cover them as well?\n>\n> Thanks.\n\nI’ve incorporated the `column.c` patch you posted in my\nnot-yet-published v2. Hopefully that was it.(? :) ) I’ll take another\nlook.\n\nv2 is finished now so maybe I’ll send it out soon.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"488416","messageId":"1c959378cf495d7a3d70d0c7bdf08cc501ed6e5d.1707679627.git.code@khaugsbakk.name","threadId":"60883","inReplyTo":"76688ed2cc20031d70823d9f5d214f42b3bd1409.1707501064.git.code@khaugsbakk.name","subject":"[PATCH v2] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-11T19:27:49Z","receivedAt":"2024-02-11T19:28:16Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"A negative padding does not make sense and can cause errors in the\nmemory allocator since it’s interpreted as an unsigned integer.\n\nDisallow negative padding. Also guard against negative padding in\n`column.c` where it is conditionally used.\n\nReported-by: Tiago Pascoal <tiago@pascoal.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    v2:\n    • Incorporate Junio’s changes (guard against negative padding in\n      `column.c`)\n    • Tweak commit message based on Junio’s analysis\n    • Use gettext for error message\n      • However I noticed that the “translation string” from `fast-import`\n        isn’t a translation string. So let’s invent a new one and use a\n        parameter so that it can be used elsewhere.\n    • Make a test\n\n builtin/column.c  |  2 ++\n column.c          |  4 ++--\n t/t9002-column.sh | 11 +++++++++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/column.c b/builtin/column.c\nindex e80218f81f9..10ff7e01668 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \tmemset(&copts, 0, sizeof(copts));\n \tcopts.padding = 1;\n \targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n+\tif (copts.padding < 0)\n+\t\tdie(_(\"%s must be non-negative\"), \"--padding\");\n \tif (argc)\n \t\tusage_with_options(builtin_column_usage, options);\n \tif (real_command || command) {\ndiff --git a/column.c b/column.c\nindex ff2f0abf399..c723428bc70 100644\n--- a/column.c\n+++ b/column.c\n@@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n \tmemset(&nopts, 0, sizeof(nopts));\n \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n-\tnopts.padding = opts ? opts->padding : 1;\n+\tnopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;\n \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n \tif (!column_active(colopts)) {\n \t\tdisplay_plain(list, \"\", \"\\n\");\n@@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\n \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n \tif (opts && opts->indent)\n \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n-\tif (opts && opts->padding)\n+\tif (opts && 0 <= opts->padding)\n \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n \n \tfflush(stdout);\ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 348cc406582..d5b98e615bc 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -196,4 +196,15 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'padding must be non-negative' '\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 <input >actual 2>&1 &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"488424","messageId":"89d32a5f-b5ab-4773-bd9f-d33b4e348e15@gmail.com","threadId":"60883","inReplyTo":"1c959378cf495d7a3d70d0c7bdf08cc501ed6e5d.1707679627.git.code@khaugsbakk.name","subject":"Re: [PATCH v2] column: disallow negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-11T22:47:54Z","receivedAt":"2024-02-11T22:48:12Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 11-feb-2024 20:27:49, Kristoffer Haugsbakk wrote:\n> A negative padding does not make sense and can cause errors in the\n> memory allocator since it’s interpreted as an unsigned integer.\n> \n> Disallow negative padding. Also guard against negative padding in\n> `column.c` where it is conditionally used.\n> \n> Reported-by: Tiago Pascoal <tiago@pascoal.net>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n> \n> Notes (series):\n>     v2:\n>     • Incorporate Junio’s changes (guard against negative padding in\n>       `column.c`)\n>     • Tweak commit message based on Junio’s analysis\n>     • Use gettext for error message\n>       • However I noticed that the “translation string” from `fast-import`\n>         isn’t a translation string. So let’s invent a new one and use a\n>         parameter so that it can be used elsewhere.\n>     • Make a test\n> \n>  builtin/column.c  |  2 ++\n>  column.c          |  4 ++--\n>  t/t9002-column.sh | 11 +++++++++++\n>  3 files changed, 15 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/column.c b/builtin/column.c\n> index e80218f81f9..10ff7e01668 100644\n> --- a/builtin/column.c\n> +++ b/builtin/column.c\n> @@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n>  \tmemset(&copts, 0, sizeof(copts));\n>  \tcopts.padding = 1;\n>  \targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n> +\tif (copts.padding < 0)\n> +\t\tdie(_(\"%s must be non-negative\"), \"--padding\");\n\nWe clearly inform the user and die.  No more OOM errors, or worse.\nGood.\n\nAnd the message avoids translation problems.  Excellent.\n\n>  \tif (argc)\n>  \t\tusage_with_options(builtin_column_usage, options);\n>  \tif (real_command || command) {\n> diff --git a/column.c b/column.c\n> index ff2f0abf399..c723428bc70 100644\n> --- a/column.c\n> +++ b/column.c\n> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n>  \tmemset(&nopts, 0, sizeof(nopts));\n>  \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n>  \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n> -\tnopts.padding = opts ? opts->padding : 1;\n> +\tnopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;\n\nThis changes what Junio proposed.  Is this on purpose?\n\nWhile we're here, I wonder if silently ignoring a negative value in\n.padding is the right thing to do.\n\nThere are several callers of print_columns():\n\nbuiltin/branch.c:           print_columns(&output, colopts, NULL);\nbuiltin/clean.c:    print_columns(&list, colopts, &copts);\nbuiltin/clean.c:    print_columns(menu_list, local_colopts, &copts);\nbuiltin/column.c:    print_columns(&list, colopts, &copts);\nhelp.c:     print_columns(&list, colopts, &copts);\nwt-status.c:       print_columns(&output, s->colopts, &copts);\n\nI haven't checked it thoroughly but it seems we don't need to add the\ncheck we're adding to builtin/column.c, to any of the other callers.\nHowever, it is possible that these or other new callers may need it in\nthe future.  If so, we should consider doing something like:\n\ndiff --git a/column.c b/column.c\nindex c723428bc7..4f870c725f 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                return;\n        assert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n\n+       if (opts && (0 <= opts->padding))\n+               BUG(\"padding must be non-negative\");\n+\n        memset(&nopts, 0, sizeof(nopts));\n        nopts.indent = opts && opts->indent ? opts->indent : \"\";\n        nopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n\n>  \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n>  \tif (!column_active(colopts)) {\n>  \t\tdisplay_plain(list, \"\", \"\\n\");\n> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\n>  \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n>  \tif (opts && opts->indent)\n>  \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n> -\tif (opts && opts->padding)\n> +\tif (opts && 0 <= opts->padding)\n\nThis also differs from Junio's changes.\n\n>  \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n>  \n>  \tfflush(stdout);\n> diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n> index 348cc406582..d5b98e615bc 100755\n> --- a/t/t9002-column.sh\n> +++ b/t/t9002-column.sh\n> @@ -196,4 +196,15 @@ EOF\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'padding must be non-negative' '\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 <input >actual 2>&1 &&\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_done\n\nOK\n\n> -- \n> 2.43.0\n> \n"},{"id":"488425","messageId":"c44117e1-00fe-4d57-aa21-adb746f4d7b4@gmail.com","threadId":"60883","inReplyTo":"89d32a5f-b5ab-4773-bd9f-d33b4e348e15@gmail.com","subject":"Re: [PATCH v2] column: disallow negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-11T23:50:12Z","receivedAt":"2024-02-11T23:50:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\n\nOn 11/2/24 23:47, Rubén Justo wrote:\n> On 11-feb-2024 20:27:49, Kristoffer Haugsbakk wrote:\n>> A negative padding does not make sense and can cause errors in the\n>> memory allocator since it’s interpreted as an unsigned integer.\n>>\n>> Disallow negative padding. Also guard against negative padding in\n>> `column.c` where it is conditionally used.\n>>\n>> Reported-by: Tiago Pascoal <tiago@pascoal.net>\n>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n>> ---\n>>\n>> Notes (series):\n>>     v2:\n>>     • Incorporate Junio’s changes (guard against negative padding in\n>>       `column.c`)\n>>     • Tweak commit message based on Junio’s analysis\n>>     • Use gettext for error message\n>>       • However I noticed that the “translation string” from `fast-import`\n>>         isn’t a translation string. So let’s invent a new one and use a\n>>         parameter so that it can be used elsewhere.\n>>     • Make a test\n>>\n>>  builtin/column.c  |  2 ++\n>>  column.c          |  4 ++--\n>>  t/t9002-column.sh | 11 +++++++++++\n>>  3 files changed, 15 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/column.c b/builtin/column.c\n>> index e80218f81f9..10ff7e01668 100644\n>> --- a/builtin/column.c\n>> +++ b/builtin/column.c\n>> @@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n>>  \tmemset(&copts, 0, sizeof(copts));\n>>  \tcopts.padding = 1;\n>>  \targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n>> +\tif (copts.padding < 0)\n>> +\t\tdie(_(\"%s must be non-negative\"), \"--padding\");\n> \n> We clearly inform the user and die.  No more OOM errors, or worse.\n> Good.\n> \n> And the message avoids translation problems.  Excellent.\n> \n>>  \tif (argc)\n>>  \t\tusage_with_options(builtin_column_usage, options);\n>>  \tif (real_command || command) {\n>> diff --git a/column.c b/column.c\n>> index ff2f0abf399..c723428bc70 100644\n>> --- a/column.c\n>> +++ b/column.c\n>> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n>>  \tmemset(&nopts, 0, sizeof(nopts));\n>>  \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n>>  \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n>> -\tnopts.padding = opts ? opts->padding : 1;\n>> +\tnopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;\n> \n> This changes what Junio proposed.  Is this on purpose?\n> \n> While we're here, I wonder if silently ignoring a negative value in\n> .padding is the right thing to do.\n> \n> There are several callers of print_columns():\n> \n> builtin/branch.c:           print_columns(&output, colopts, NULL);\n> builtin/clean.c:    print_columns(&list, colopts, &copts);\n> builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);\n> builtin/column.c:    print_columns(&list, colopts, &copts);\n> help.c:     print_columns(&list, colopts, &copts);\n> wt-status.c:       print_columns(&output, s->colopts, &copts);\n> \n> I haven't checked it thoroughly but it seems we don't need to add the\n> check we're adding to builtin/column.c, to any of the other callers.\n> However, it is possible that these or other new callers may need it in\n> the future.  If so, we should consider doing something like:\n> \n> diff --git a/column.c b/column.c\n> index c723428bc7..4f870c725f 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>                 return;\n>         assert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n> \n> +       if (opts && (0 <= opts->padding))\n\nOops.  Of course, I mean:\n+       if (opts && (0 > opts->padding))\n\nSorry.\n\n> +               BUG(\"padding must be non-negative\");\n> +\n>         memset(&nopts, 0, sizeof(nopts));\n>         nopts.indent = opts && opts->indent ? opts->indent : \"\";\n>         nopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n> \n>>  \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n>>  \tif (!column_active(colopts)) {\n>>  \t\tdisplay_plain(list, \"\", \"\\n\");\n>> @@ -373,7 +373,7 @@ int run_column_filter(int colopts, const struct column_options *opts)\n>>  \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n>>  \tif (opts && opts->indent)\n>>  \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n>> -\tif (opts && opts->padding)\n>> +\tif (opts && 0 <= opts->padding)\n> \n> This also differs from Junio's changes.\n> \n>>  \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n>>  \n>>  \tfflush(stdout);\n>> diff --git a/t/t9002-column.sh b/t/t9002-column.sh\n>> index 348cc406582..d5b98e615bc 100755\n>> --- a/t/t9002-column.sh\n>> +++ b/t/t9002-column.sh\n>> @@ -196,4 +196,15 @@ EOF\n>>  \ttest_cmp expected actual\n>>  '\n>>  \n>> +test_expect_success 'padding must be non-negative' '\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 <input >actual 2>&1 &&\n>> +\ttest_cmp expected actual\n>> +'\n>> +\n>>  test_done\n> \n> OK\n> \n>> -- \n>> 2.43.0\n>>\n"},{"id":"488428","messageId":"9582509b-9569-4321-a2b0-e5b33c2d550c@app.fastmail.com","threadId":"60883","inReplyTo":"89d32a5f-b5ab-4773-bd9f-d33b4e348e15@gmail.com","subject":"Re: [PATCH v2] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-12T07:05:40Z","receivedAt":"2024-02-12T07:06:03Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hey, thanks for the review\n\nOn Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:\n>>  \tif (argc)\n>>  \t\tusage_with_options(builtin_column_usage, options);\n>>  \tif (real_command || command) {\n>> diff --git a/column.c b/column.c\n>> index ff2f0abf399..c723428bc70 100644\n>> --- a/column.c\n>> +++ b/column.c\n>> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n>>  \tmemset(&nopts, 0, sizeof(nopts));\n>>  \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n>>  \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n>> -\tnopts.padding = opts ? opts->padding : 1;\n>> +\tnopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;\n>\n> This changes what Junio proposed.  Is this on purpose?\n\nYes https://lore.kernel.org/git/3380df68-83fb-417b-a490-71614edc342f@app.fastmail.com/T/#m63ca728414def19b7a0c83ec76a8c1f2de68ffbb\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"488447","messageId":"xmqqh6id8wqi.fsf@gitster.g","threadId":"60883","inReplyTo":"3380df68-83fb-417b-a490-71614edc342f@app.fastmail.com","subject":"Re: git column fails (or crashes) if padding is negative","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-12T16:37:25Z","receivedAt":"2024-02-12T16:37:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n\n> On Fri, Feb 9, 2024, at 18:57, Junio C Hamano wrote:\n>>  builtin/column.c | 2 ++\n>>  column.c         | 4 ++--\n>>  2 files changed, 4 insertions(+), 2 deletions(-)\n>>\n>> […]\n>> diff --git c/column.c w/column.c\n>> index ff2f0abf39..9cc703832a 100644\n>> --- c/column.c\n>> +++ w/column.c\n>> @@ -189,7 +189,7 @@ void print_columns(const struct string_list *list,\n>> unsigned int colopts,\n>>  \tmemset(&nopts, 0, sizeof(nopts));\n>>  \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n>>  \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n>> -\tnopts.padding = opts ? opts->padding : 1;\n>> +\tnopts.padding = (opts && 0 < opts->padding) ? opts->padding : 1;\n>\n> If these two are meant to check the same condition as in\n> `builtin/column.c`, shouldn’t it be `0 <= opts->padding`?\n\nGood eyes.  Otherwise we lose the ability to set the padding to 0.\n"},{"id":"488453","messageId":"09b1ab48-b58f-458c-89f5-0c419d92f61a@app.fastmail.com","threadId":"60883","inReplyTo":"89d32a5f-b5ab-4773-bd9f-d33b4e348e15@gmail.com","subject":"Re: [PATCH v2] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-12T16:50:54Z","receivedAt":"2024-02-12T16:51:17Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:\n> While we're here, I wonder if silently ignoring a negative value in\n> .padding is the right thing to do.\n>\n> There are several callers of print_columns():\n>\n> builtin/branch.c:           print_columns(&output, colopts, NULL);\n> builtin/clean.c:    print_columns(&list, colopts, &copts);\n> builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);\n> builtin/column.c:    print_columns(&list, colopts, &copts);\n> help.c:     print_columns(&list, colopts, &copts);\n> wt-status.c:       print_columns(&output, s->colopts, &copts);\n>\n> I haven't checked it thoroughly but it seems we don't need to add the\n> check we're adding to builtin/column.c, to any of the other callers.\n> However, it is possible that these or other new callers may need it in\n> the future.  If so, we should consider doing something like:\n>\n> diff --git a/column.c b/column.c\n> index c723428bc7..4f870c725f 100644\n> --- a/column.c\n> +++ b/column.c\n> @@ -186,6 +186,9 @@ void print_columns(const struct string_list *list,\n> unsigned int colopts,\n>                 return;\n>         assert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n>\n> +       if (opts && (0 <= opts->padding))\n> +               BUG(\"padding must be non-negative\");\n> +\n\nSure, I could add a `BUG` for `0 > opts->padding` in v3.\n"},{"id":"488473","messageId":"effd06fb-8344-4476-b5d5-dcb9f6fff692@gmail.com","threadId":"60883","inReplyTo":"09b1ab48-b58f-458c-89f5-0c419d92f61a@app.fastmail.com","subject":"Re: [PATCH v2] column: disallow negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-12T21:28:57Z","receivedAt":"2024-02-12T21:29:02Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12-feb-2024 17:50:54, Kristoffer Haugsbakk wrote:\n> On Sun, Feb 11, 2024, at 23:47, Rubén Justo wrote:\n> > While we're here, I wonder if silently ignoring a negative value in\n> > .padding is the right thing to do.\n> >\n> > There are several callers of print_columns():\n> >\n> > builtin/branch.c:           print_columns(&output, colopts, NULL);\n> > builtin/clean.c:    print_columns(&list, colopts, &copts);\n> > builtin/clean.c:    print_columns(menu_list, local_colopts, &copts);\n> > builtin/column.c:    print_columns(&list, colopts, &copts);\n> > help.c:     print_columns(&list, colopts, &copts);\n> > wt-status.c:       print_columns(&output, s->colopts, &copts);\n> >\n> > I haven't checked it thoroughly but it seems we don't need to add the\n> > check we're adding to builtin/column.c, to any of the other callers.\n> > However, it is possible that these or other new callers may need it in\n> > the future.  If so, we should consider doing something like:\n> >\n> > diff --git a/column.c b/column.c\n> > index c723428bc7..4f870c725f 100644\n> > --- a/column.c\n> > +++ b/column.c\n> > @@ -186,6 +186,9 @@ void print_columns(const struct string_list *list,\n> > unsigned int colopts,\n> >                 return;\n> >         assert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n> >\n> > +       if (opts && (0 > opts->padding))\n\n;-) (fixed)\n\n> > +               BUG(\"padding must be non-negative\");\n> > +\n> \n> Sure, I could add a `BUG` for `0 > opts->padding` in v3.\n\nThank you for considering it.\n"},{"id":"488532","messageId":"cover.1707839454.git.code@khaugsbakk.name","threadId":"60883","inReplyTo":"1c959378cf495d7a3d70d0c7bdf08cc501ed6e5d.1707679627.git.code@khaugsbakk.name","subject":"[PATCH v3 0/2] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-13T16:01:19Z","receivedAt":"2024-02-13T16:02:01Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Fix bug in git-column(1): a user can pass a negative `padding` which\ncauses issues inside the memory allocator.\n\n§ Changes in v3\n\nIncorporate Ruben’s suggestion about guarding against negative padding\nwith `BUG` in `column.c` (not `builtin/column.c`). This then supersedes\nJunio’s extra conditional checks since they are no longer needed. The\nseries gets split into two patches.\n\nCc: Tiago Pascoal <tiago@pascoal.net>\nCc: Chris Torek <chris.torek@gmail.com>\nCc: Junio C Hamano <gitster@pobox.com>\nCc: Rubén Justo <rjusto@gmail.com>\n\nKristoffer Haugsbakk (2):\n  column: disallow negative padding\n  column: guard against negative padding\n\n builtin/column.c  |  2 ++\n column.c          |  4 ++++\n t/t9002-column.sh | 11 +++++++++++\n 3 files changed, 17 insertions(+)\n\nRange-diff against v2:\n1:  1c959378cf4 ! 1:  4cac42ca6f8 column: disallow negative padding\n    @@ Commit message\n         A negative padding does not make sense and can cause errors in the\n         memory allocator since it’s interpreted as an unsigned integer.\n     \n    -    Disallow negative padding. Also guard against negative padding in\n    -    `column.c` where it is conditionally used.\n    -\n         Reported-by: Tiago Pascoal <tiago@pascoal.net>\n    -    Helped-by: Junio C Hamano <gitster@pobox.com>\n         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n     \n    -\n    - ## Notes (series) ##\n    -    v2:\n    -    • Incorporate Junio’s changes (guard against negative padding in\n    -      `column.c`)\n    -    • Tweak commit message based on Junio’s analysis\n    -    • Use gettext for error message\n    -      • However I noticed that the “translation string” from `fast-import`\n    -        isn’t a translation string. So let’s invent a new one and use a\n    -        parameter so that it can be used elsewhere.\n    -    • Make a test\n    -\n      ## builtin/column.c ##\n     @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix)\n      \tmemset(&copts, 0, sizeof(copts));\n    @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix\n      \t\tusage_with_options(builtin_column_usage, options);\n      \tif (real_command || command) {\n     \n    - ## column.c ##\n    -@@ column.c: void print_columns(const struct string_list *list, unsigned int colopts,\n    - \tmemset(&nopts, 0, sizeof(nopts));\n    - \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n    - \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n    --\tnopts.padding = opts ? opts->padding : 1;\n    -+\tnopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;\n    - \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n    - \tif (!column_active(colopts)) {\n    - \t\tdisplay_plain(list, \"\", \"\\n\");\n    -@@ column.c: int run_column_filter(int colopts, const struct column_options *opts)\n    - \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n    - \tif (opts && opts->indent)\n    - \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n    --\tif (opts && opts->padding)\n    -+\tif (opts && 0 <= opts->padding)\n    - \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n    - \n    - \tfflush(stdout);\n    -\n      ## t/t9002-column.sh ##\n     @@ t/t9002-column.sh: EOF\n      \ttest_cmp expected actual\n-:  ----------- > 2:  9355fc98e3d column: guard against negative padding\n-- \n2.43.0\n\n"},{"id":"488533","messageId":"4cac42ca6f8ade5e0200b9f16f1627f0796411d1.1707839454.git.code@khaugsbakk.name","threadId":"60883","inReplyTo":"cover.1707839454.git.code@khaugsbakk.name","subject":"[PATCH v3 1/2] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-13T16:01:20Z","receivedAt":"2024-02-13T16:02:04Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"A negative padding does not make sense and can cause errors in the\nmemory allocator since it’s interpreted as an unsigned integer.\n\nReported-by: Tiago Pascoal <tiago@pascoal.net>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n builtin/column.c  |  2 ++\n t/t9002-column.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/builtin/column.c b/builtin/column.c\nindex e80218f81f9..10ff7e01668 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -45,6 +45,8 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \tmemset(&copts, 0, sizeof(copts));\n \tcopts.padding = 1;\n \targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n+\tif (copts.padding < 0)\n+\t\tdie(_(\"%s must be non-negative\"), \"--padding\");\n \tif (argc)\n \t\tusage_with_options(builtin_column_usage, options);\n \tif (real_command || command) {\ndiff --git a/t/t9002-column.sh b/t/t9002-column.sh\nindex 348cc406582..d5b98e615bc 100755\n--- a/t/t9002-column.sh\n+++ b/t/t9002-column.sh\n@@ -196,4 +196,15 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'padding must be non-negative' '\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 <input >actual 2>&1 &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"488534","messageId":"9355fc98e3dac5768ecaf9e179be2f7a0e74d633.1707839454.git.code@khaugsbakk.name","threadId":"60883","inReplyTo":"cover.1707839454.git.code@khaugsbakk.name","subject":"[PATCH v3 2/2] column: guard against negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-13T16:01:21Z","receivedAt":"2024-02-13T16:02:06Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Make sure that client code can’t pass in a negative padding by accident.\n\nSuggested-by: Rubén Justo <rjusto@gmail.com>\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    Apparently these are the only publicly-visible functions that use this\n    struct according to `column.h`.\n\n column.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/column.c b/column.c\nindex ff2f0abf399..50bbccc92ee 100644\n--- a/column.c\n+++ b/column.c\n@@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n {\n \tstruct column_options nopts;\n \n+\tif (opts && (0 > opts->padding))\n+\t\tBUG(\"padding must be non-negative\");\n \tif (!list->nr)\n \t\treturn;\n \tassert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n@@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)\n {\n \tstruct strvec *argv;\n \n+\tif (opts && (0 > opts->padding))\n+\t\tBUG(\"padding must be non-negative\");\n \tif (fd_out != -1)\n \t\treturn -1;\n \n-- \n2.43.0\n\n"},{"id":"488540","messageId":"xmqqcyt08fa1.fsf@gitster.g","threadId":"60883","inReplyTo":"9355fc98e3dac5768ecaf9e179be2f7a0e74d633.1707839454.git.code@khaugsbakk.name","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T17:06:46Z","receivedAt":"2024-02-13T17:06:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n\n> Make sure that client code can’t pass in a negative padding by accident.\n>\n> Suggested-by: Rubén Justo <rjusto@gmail.com>\n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n>\n> Notes (series):\n>     Apparently these are the only publicly-visible functions that use this\n>     struct according to `column.h`.\n>\n>  column.c | 4 ++++\n>  1 file changed, 4 insertions(+)\n>\n> diff --git a/column.c b/column.c\n> index ff2f0abf399..50bbccc92ee 100644\n> --- a/column.c\n> +++ b/column.c\n> @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n>  {\n>  \tstruct column_options nopts;\n>  \n> +\tif (opts && (0 > opts->padding))\n> +\t\tBUG(\"padding must be non-negative\");\n\nThe only two current callers may happen to be \"git branch\" that\npasses NULL as opts, and \"git clean\" that passes 2 in opts->padding,\nso this BUG() will not trigger.  Once we add new callers to this\nfunction, or update the current callers, this safety start to matter.\n\nThe actual breakage from a negative padding happens in layout(),\nso another option would be to have this guard there, which will\nprotect us from having new callers of that function as well, or\nits caller display_table(), but these have only one caller each,\nso having the guard print_columns() here, that is the closest to\nthe callers would be fine.\n\n>  \tif (!list->nr)\n>  \t\treturn;\n>  \tassert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n> @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)\n>  {\n>  \tstruct strvec *argv;\n>  \n> +\tif (opts && (0 > opts->padding))\n> +\t\tBUG(\"padding must be non-negative\");\n\nThis one happens to be safe currently because \"git tag\" passes 2 in\nopts->padding, but I do not think this is needed.\n\nWe will pass these through to \"git column\" and the negative padding\nwill be caught as an error there anyway, no?  So whether \"git tag\"\nis updated or a new caller of run_column_filter() is added, the\ndeveloper will already notice it (and they will have to protect\nthemselves just like the [1/2] of your series did for \"git column\"\nitself).\n\n>  \tif (fd_out != -1)\n>  \t\treturn -1;\n"},{"id":"488563","messageId":"69f60c3a-ff47-4cb9-a229-6c5a36e7d9fa@gmail.com","threadId":"60883","inReplyTo":"xmqqcyt08fa1.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-13T18:39:51Z","receivedAt":"2024-02-13T18:39:55Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 13-feb-2024 09:06:46, Junio C Hamano wrote:\n> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:\n> \n> > Make sure that client code can’t pass in a negative padding by accident.\n> >\n> > Suggested-by: Rubén Justo <rjusto@gmail.com>\n> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> > ---\n> >\n> > Notes (series):\n> >     Apparently these are the only publicly-visible functions that use this\n> >     struct according to `column.h`.\n> >\n> >  column.c | 4 ++++\n> >  1 file changed, 4 insertions(+)\n> >\n> > diff --git a/column.c b/column.c\n> > index ff2f0abf399..50bbccc92ee 100644\n> > --- a/column.c\n> > +++ b/column.c\n> > @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts,\n> >  {\n> >  \tstruct column_options nopts;\n> >  \n> > +\tif (opts && (0 > opts->padding))\n> > +\t\tBUG(\"padding must be non-negative\");\n> \n> The only two current callers may happen to be \"git branch\" that\n> passes NULL as opts, and \"git clean\" that passes 2 in opts->padding,\n> so this BUG() will not trigger.  Once we add new callers to this\n> function, or update the current callers, this safety start to matter.\n> \n> The actual breakage from a negative padding happens in layout(),\n> so another option would be to have this guard there, which will\n> protect us from having new callers of that function as well, or\n> its caller display_table(), but these have only one caller each,\n> so having the guard print_columns() here, that is the closest to\n> the callers would be fine.\n> \n> >  \tif (!list->nr)\n> >  \t\treturn;\n> >  \tassert((colopts & COL_ENABLE_MASK) != COL_AUTO);\n> > @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts)\n> >  {\n> >  \tstruct strvec *argv;\n> >  \n> > +\tif (opts && (0 > opts->padding))\n> > +\t\tBUG(\"padding must be non-negative\");\n> \n> This one happens to be safe currently because \"git tag\" passes 2 in\n> opts->padding, but I do not think this is needed.\n\nAt first glance, I also thought this was not necessary.\n\nHowever, callers of run_column_filter() might forget to check the return\nvalue, and the BUG() triggered by the underlying process could be buried\nand ignored.  Having the BUG() here, in the same process, makes it more\nnoticeable.\n\nBased on this, I'm not opposed to this change.\n"},{"id":"488564","messageId":"48b96426-9231-4e80-b55d-628dd8847337@gmail.com","threadId":"60883","inReplyTo":"cover.1707839454.git.code@khaugsbakk.name","subject":"Re: [PATCH v3 0/2] column: disallow negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-13T19:27:25Z","receivedAt":"2024-02-13T19:27:29Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 13-feb-2024 17:01:19, Kristoffer Haugsbakk wrote:\n\n> The series gets split into two patches.\n\nVery good.\n\n> \n> Cc: Tiago Pascoal <tiago@pascoal.net>\n> Cc: Chris Torek <chris.torek@gmail.com>\n> Cc: Junio C Hamano <gitster@pobox.com>\n> Cc: Rubén Justo <rjusto@gmail.com>\n> \n> Kristoffer Haugsbakk (2):\n>   column: disallow negative padding\n>   column: guard against negative padding\n> \n>  builtin/column.c  |  2 ++\n>  column.c          |  4 ++++\n>  t/t9002-column.sh | 11 +++++++++++\n>  3 files changed, 17 insertions(+)\n> \n> Range-diff against v2:\n> 1:  1c959378cf4 ! 1:  4cac42ca6f8 column: disallow negative padding\n>     @@ Commit message\n>          A negative padding does not make sense and can cause errors in the\n>          memory allocator since it’s interpreted as an unsigned integer.\n>      \n>     -    Disallow negative padding. Also guard against negative padding in\n>     -    `column.c` where it is conditionally used.\n>     -\n>          Reported-by: Tiago Pascoal <tiago@pascoal.net>\n>     -    Helped-by: Junio C Hamano <gitster@pobox.com>\n>          Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n>      \n>     -\n>     - ## Notes (series) ##\n>     -    v2:\n>     -    • Incorporate Junio’s changes (guard against negative padding in\n>     -      `column.c`)\n>     -    • Tweak commit message based on Junio’s analysis\n>     -    • Use gettext for error message\n>     -      • However I noticed that the “translation string” from `fast-import`\n>     -        isn’t a translation string. So let’s invent a new one and use a\n>     -        parameter so that it can be used elsewhere.\n>     -    • Make a test\n>     -\n>       ## builtin/column.c ##\n>      @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix)\n>       \tmemset(&copts, 0, sizeof(copts));\n>     @@ builtin/column.c: int cmd_column(int argc, const char **argv, const char *prefix\n>       \t\tusage_with_options(builtin_column_usage, options);\n>       \tif (real_command || command) {\n>      \n>     - ## column.c ##\n>     -@@ column.c: void print_columns(const struct string_list *list, unsigned int colopts,\n>     - \tmemset(&nopts, 0, sizeof(nopts));\n>     - \tnopts.indent = opts && opts->indent ? opts->indent : \"\";\n>     - \tnopts.nl = opts && opts->nl ? opts->nl : \"\\n\";\n>     --\tnopts.padding = opts ? opts->padding : 1;\n>     -+\tnopts.padding = (opts && 0 <= opts->padding) ? opts->padding : 1;\n>     - \tnopts.width = opts && opts->width ? opts->width : term_columns() - 1;\n>     - \tif (!column_active(colopts)) {\n>     - \t\tdisplay_plain(list, \"\", \"\\n\");\n>     -@@ column.c: int run_column_filter(int colopts, const struct column_options *opts)\n>     - \t\tstrvec_pushf(argv, \"--width=%d\", opts->width);\n>     - \tif (opts && opts->indent)\n>     - \t\tstrvec_pushf(argv, \"--indent=%s\", opts->indent);\n>     --\tif (opts && opts->padding)\n>     -+\tif (opts && 0 <= opts->padding)\n>     - \t\tstrvec_pushf(argv, \"--padding=%d\", opts->padding);\n>     - \n>     - \tfflush(stdout);\n>     -\n>       ## t/t9002-column.sh ##\n>      @@ t/t9002-column.sh: EOF\n>       \ttest_cmp expected actual\n> -:  ----------- > 2:  9355fc98e3d column: guard against negative padding\n> -- \n> 2.43.0\n> \n\nThe BUG() in run_column_filter() may be questionable, but overall this\nv3 LGTM.\n\nThanks Kristoffer for your work.  And also thanks to Tiago for\nreporting.\n\n\n    * P.D. *\n    \n    Thinking about this in a more general way, I've found that this kind\n    of error has hit us several times:\n    \n      - 953aa54e1a (pack-objects: clamp negative window size to 0, 2021-05-01)\n      - 6d52b6a5df (pack-objects: clamp negative depth to 0, 2021-05-01)\n    \n    Maybe the source of this error is how easy is to forget that\n    OPT_INTEGER can accept negative values (after all, that's what an\n    integer is).\n    \n    There are not many users of OPT_INTEGER, and a quick check gives me\n    the impression (maybe wrong...) that many of them do not expect\n    negative values.\n    \n    Maybe we should consider having an OPT_INTEGER that fails if the\n    value supplied is negative.  Ideally, some kind of opt-in machinery\n    could be desirable, I think, for example to include/exclude:\n\n    \t- negative values\n    \t- \"0\"  ( may not be a desired value )\n    \t- \"-1\" ( may have some special meaning )\n    \t- ...\n    \n    I'll leave the idea here, just in case it inspires someone.  Thank\n    you.\n"},{"id":"488565","messageId":"xmqqle7o5f34.fsf@gitster.g","threadId":"60883","inReplyTo":"69f60c3a-ff47-4cb9-a229-6c5a36e7d9fa@gmail.com","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T19:39:11Z","receivedAt":"2024-02-13T19:39:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> This one happens to be safe currently because \"git tag\" passes 2 in\n>> opts->padding, but I do not think this is needed.\n>\n> At first glance, I also thought this was not necessary.\n>\n> However, callers of run_column_filter() might forget to check the return\n> value, and the BUG() triggered by the underlying process could be buried\n> and ignored.  Having the BUG() here, in the same process, makes it more\n> noticeable.\n\nThe point of BUG() is to help developers catch the silly breakage\nbefore it excapes from the lab, and we can expect these careless\ndevelopers to ignore the return value.  But \"column --padding=-1\"\ninvoked as a subprocess will show a human-readable error message\nto such a developer, so it is less important than the BUG() in the\nother place.\n\nThere is no black or white decision, but this one is much less\ndarker gray than the other one is.\n"},{"id":"488570","messageId":"b9ca1ab7-f8f6-4fe0-885a-51728d9ec708@gmail.com","threadId":"60883","inReplyTo":"xmqqle7o5f34.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-13T19:56:23Z","receivedAt":"2024-02-13T19:56:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 13-feb-2024 11:39:11, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> >> This one happens to be safe currently because \"git tag\" passes 2 in\n> >> opts->padding, but I do not think this is needed.\n> >\n> > At first glance, I also thought this was not necessary.\n> >\n> > However, callers of run_column_filter() might forget to check the return\n> > value, and the BUG() triggered by the underlying process could be buried\n> > and ignored.  Having the BUG() here, in the same process, makes it more\n> > noticeable.\n> \n> The point of BUG() is to help developers catch the silly breakage\n> before it excapes from the lab, and we can expect these careless\n> developers to ignore the return value.  But \"column --padding=-1\"\n> invoked as a subprocess will show a human-readable error message\n> to such a developer, so it is less important than the BUG() in the\n> other place.\n> \n> There is no black or white decision, but this one is much less\n> darker gray than the other one is.\n\nI've checked this, without that BUG(), and the result has not been\npretty:\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 37473ac21f..e15dfa73d2 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n                if (column_active(colopts)) {\n                        struct column_options copts;\n                        memset(&copts, 0, sizeof(copts));\n-                       copts.padding = 2;\n+                       copts.padding = -1;\n                        run_column_filter(colopts, &copts);\n                }\n                filter.name_patterns = argv;\n\nI can imagine a future change that opens that current \"2\" to the user.\nAnd the possible report from a user who tries \"-1\" would not be easy.\n\nBut I agree with you, that BUG() does not leave a good taste in the\nmouth.\n\nMaybe we should refactor run_column_filter(), I don't know, but I think\nthat is outside of the scope of this series.\n"},{"id":"488576","messageId":"dc1be818-e793-44f6-98dd-b159e043da28@app.fastmail.com","threadId":"60883","inReplyTo":"48b96426-9231-4e80-b55d-628dd8847337@gmail.com","subject":"Re: [PATCH v3 0/2] column: disallow negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-13T20:32:32Z","receivedAt":"2024-02-13T20:32:54Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Feb 13, 2024, at 20:27, Rubén Justo wrote:\n>     * P.D. *\n>\n>     Thinking about this in a more general way, I've found that this kind\n>     of error has hit us several times:\n>\n>       - 953aa54e1a (pack-objects: clamp negative window size to 0, 2021-05-01)\n>       - 6d52b6a5df (pack-objects: clamp negative depth to 0, 2021-05-01)\n>\n>     Maybe the source of this error is how easy is to forget that\n>     OPT_INTEGER can accept negative values (after all, that's what an\n>     integer is).\n>\n>     There are not many users of OPT_INTEGER, and a quick check gives me\n>     the impression (maybe wrong...) that many of them do not expect\n>     negative values.\n>\n>     Maybe we should consider having an OPT_INTEGER that fails if the\n>     value supplied is negative.  Ideally, some kind of opt-in machinery\n>     could be desirable, I think, for example to include/exclude:\n>\n>     \t- negative values\n>     \t- \"0\"  ( may not be a desired value )\n>     \t- \"-1\" ( may have some special meaning )\n>     \t- ...\n>\n>     I'll leave the idea here, just in case it inspires someone.  Thank\n>     you.\n\nThanks to both for providing a wider perspective on guarding against\nsuch bugs.\n\nAnd this is an excellent point. I don’t know anything about the opt-args\nimplementation but it would be great to guard against user-supplied\nvalues through the option parsing library.\n\nCheers\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"488577","messageId":"6446a48c-4b9a-4095-9083-071f95ce3b84@app.fastmail.com","threadId":"60883","inReplyTo":"b9ca1ab7-f8f6-4fe0-885a-51728d9ec708@gmail.com","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-13T20:35:44Z","receivedAt":"2024-02-13T20:36:06Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Feb 13, 2024, at 20:56, Rubén Justo wrote:\n> On 13-feb-2024 11:39:11, Junio C Hamano wrote:\n>> Rubén Justo <rjusto@gmail.com> writes:\n>> The point of BUG() is to help developers catch the silly breakage\n>> before it excapes from the lab, and we can expect these careless\n>> developers to ignore the return value.  But \"column --padding=-1\"\n>> invoked as a subprocess will show a human-readable error message\n>> to such a developer, so it is less important than the BUG() in the\n>> other place.\n>>\n>> There is no black or white decision, but this one is much less\n>> darker gray than the other one is.\n>\n> I've checked this, without that BUG(), and the result has not been\n> pretty:\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 37473ac21f..e15dfa73d2 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>                 if (column_active(colopts)) {\n>                         struct column_options copts;\n>                         memset(&copts, 0, sizeof(copts));\n> -                       copts.padding = 2;\n> +                       copts.padding = -1;\n>                         run_column_filter(colopts, &copts);\n>                 }\n>                 filter.name_patterns = argv;\n>\n> I can imagine a future change that opens that current \"2\" to the user.\n> And the possible report from a user who tries \"-1\" would not be easy.\n>\n> But I agree with you, that BUG() does not leave a good taste in the\n> mouth.\n>\n> Maybe we should refactor run_column_filter(), I don't know, but I think\n> that is outside of the scope of this series.\n\nThanks for trying that out—some very topical testing!\n\nI will take the night to think about v4. But I will defer to the\nreviewers’ judgement on the scope of this series/change.\n\n(All I know is that it can be tricky balancing such defensive checks\nwith readability and maintanability.)\n\nThanks\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"488596","messageId":"xmqqo7ck3wum.fsf@gitster.g","threadId":"60883","inReplyTo":"dc1be818-e793-44f6-98dd-b159e043da28@app.fastmail.com","subject":"Re: [PATCH v3 0/2] column: disallow negative padding","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T20:58:25Z","receivedAt":"2024-02-13T20:58:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n\n>>     There are not many users of OPT_INTEGER, and a quick check gives me\n>>     the impression (maybe wrong...) that many of them do not expect\n>>     negative values.\n>>\n>>     Maybe we should consider having an OPT_INTEGER that fails if the\n>>     value supplied is negative.  Ideally, some kind of opt-in machinery\n>>     could be desirable, I think, for example to include/exclude:\n>>\n>>     \t- negative values\n>>     \t- \"0\"  ( may not be a desired value )\n>>     \t- \"-1\" ( may have some special meaning )\n>>     \t- ...\n>>\n>>     I'll leave the idea here, just in case it inspires someone.  Thank\n>>     you.\n\nInteresting.\n\nI wonder if there is a correlation between \"never negative\" and\n\"handy if it took scale unit (like 2k to mean 2048)\"?  If so,\nperhaps we can replace those that use OPT_INTEGER to use\nOPT_MAGNITUDE instead.\n\nThanks.\n"},{"id":"488597","messageId":"xmqqjzn83wsr.fsf@gitster.g","threadId":"60883","inReplyTo":"6446a48c-4b9a-4095-9083-071f95ce3b84@app.fastmail.com","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T20:59:32Z","receivedAt":"2024-02-13T20:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n\n> (All I know is that it can be tricky balancing such defensive checks\n> with readability and maintanability.)\n\nPersonally I think v3 is a good enough place to stop and we are now\nentering into the realm of diminishing returns.\n\nThanks.\n"},{"id":"488604","messageId":"8acde766-e2cd-4901-b665-f677cd15295d@gmail.com","threadId":"60883","inReplyTo":"xmqqle7o5f34.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] column: guard against negative padding","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-13T23:25:46Z","receivedAt":"2024-02-13T23:25:51Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 13-feb-2024 11:39:11, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> >> This one happens to be safe currently because \"git tag\" passes 2 in\n> >> opts->padding, but I do not think this is needed.\n> >\n> > At first glance, I also thought this was not necessary.\n> >\n> > However, callers of run_column_filter() might forget to check the return\n> > value, and the BUG() triggered by the underlying process could be buried\n> > and ignored.  Having the BUG() here, in the same process, makes it more\n> > noticeable.\n> \n> The point of BUG() is to help developers catch the silly breakage\n> before it excapes from the lab, and we can expect these careless\n> developers to ignore the return value.  But \"column --padding=-1\"\n> invoked as a subprocess will show a human-readable error message\n> to such a developer, so it is less important than the BUG() in the\n> other place.\n\nThinking again about this; you are right.  That BUG() in\nrun_column_filter() does not make sense in this series.\n\nIt is addressing a different error, and perhaps a solution could be:\n\n--- >8 ---\nSubject: [PATCH] tag: error when git-column fails\n\nIf the user asks for the list of tags to be displayed in columns\n(\"--columns\"), a child git-column process is used to format the output\nas expected.\n\nIn a rare situation where we encounter a problem spawning that child\nprocess, we will work erroneously.\n\nMake noticeable we're having a problem executing git-column, so the user\ncan act accordingly.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/tag.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 37473ac21f..30532b76d5 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tstruct column_options copts;\n \t\t\tmemset(&copts, 0, sizeof(copts));\n \t\t\tcopts.padding = 2;\n-\t\t\trun_column_filter(colopts, &copts);\n+\t\t\tif (run_column_filter(colopts, &copts))\n+\t\t\t\tdie(_(\"could not start 'git column'\")\n \t\t}\n \t\tfilter.name_patterns = argv;\n \t\tret = list_tags(&filter, sorting, &format);\n-- \n2.43.0\n"},{"id":"488605","messageId":"59df085d-0de8-45b1-9b8b-c69e91e56a1f@gmail.com","threadId":"60883","inReplyTo":"8acde766-e2cd-4901-b665-f677cd15295d@gmail.com","subject":"[PATCH] tag: error when git-column fails","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-02-13T23:36:38Z","receivedAt":"2024-02-13T23:36:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"If the user asks for the list of tags to be displayed in columns\n(\"--columns\"), a child git-column process is used to format the output\nas expected.\n\nIn a rare situation where we encounter a problem spawning that child\nprocess, we will work erroneously.\n\nMake noticeable we're having a problem executing git-column, so the user\ncan act accordingly.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/tag.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 37473ac21f..19a7e06bf4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tstruct column_options copts;\n \t\t\tmemset(&copts, 0, sizeof(copts));\n \t\t\tcopts.padding = 2;\n-\t\t\trun_column_filter(colopts, &copts);\n+\t\t\tif (run_column_filter(colopts, &copts))\n+\t\t\t\tdie(_(\"could not start 'git column'\"));\n \t\t}\n \t\tfilter.name_patterns = argv;\n \t\tret = list_tags(&filter, sorting, &format);\n-- \n2.43.0\n"},{"id":"488608","messageId":"xmqq1q9f25ga.fsf@gitster.g","threadId":"60883","inReplyTo":"59df085d-0de8-45b1-9b8b-c69e91e56a1f@gmail.com","subject":"Re: [PATCH] tag: error when git-column fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-14T01:35:33Z","receivedAt":"2024-02-14T01:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> @@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\t\tstruct column_options copts;\n>  \t\t\tmemset(&copts, 0, sizeof(copts));\n>  \t\t\tcopts.padding = 2;\n> -\t\t\trun_column_filter(colopts, &copts);\n> +\t\t\tif (run_column_filter(colopts, &copts))\n> +\t\t\t\tdie(_(\"could not start 'git column'\"));\n\nNice.  This obvious omission should have been here from the day one.\n\nWill queue.  Thanks.\n\n>  \t\t}\n>  \t\tfilter.name_patterns = argv;\n>  \t\tret = list_tags(&filter, sorting, &format);\n"}]}