{"thread":{"id":"62874","subject":"[PATCH] apply: detect overflow when parsing hunk header","startedAt":"2025-01-30T11:08:34Z","lastAt":"2025-01-30T22:17:53Z","messageCount":2,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511487","messageId":"pull.1858.git.1738235310815.gitgitgadget@gmail.com","threadId":"62874","inReplyTo":null,"subject":"[PATCH] apply: detect overflow when parsing hunk header","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-30T11:08:30Z","receivedAt":"2025-01-30T11:08:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n\"git apply\" uses strtoul() to parse the numbers in the hunk header but\nsilently ignores overflows. As LONG_MAX is a legitimate return value for\nstrtoul() we need to set errno to zero before the call to strtoul() and\ncheck that it is still zero afterwards. The error message we display is\nnot particularly helpful as it does not say what was wrong.  However, it\nseems pretty unlikely that users are going to trigger this error in\npractice and we can always improve it later if needed.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    apply: detect overflow when parsing hunk header\n    \n    We should do something similar in \"git add -p\" but I'll wait to see what\n    happens with\n    https://lore.kernel.org/git/20250126125638.3089-2-soekkle@freenet.de/\n    first\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1858%2Fphillipwood%2Fapply-detect-hunk-header-overflow-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1858/phillipwood/apply-detect-hunk-header-overflow-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1858\n\n apply.c               |  3 +++\n t/t4100-apply-stat.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/apply.c b/apply.c\nindex 4a7b6120ac8..b124678b93b 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1423,7 +1423,10 @@ static int parse_num(const char *line, unsigned long *p)\n \n \tif (!isdigit(*line))\n \t\treturn 0;\n+\terrno = 0;\n \t*p = strtoul(line, &ptr, 10);\n+\tif (errno)\n+\t\treturn 0;\n \treturn ptr - line;\n }\n \ndiff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh\nindex 146e73d8f55..a5664f3eb3c 100755\n--- a/t/t4100-apply-stat.sh\n+++ b/t/t4100-apply-stat.sh\n@@ -38,4 +38,17 @@ incomplete (1)\n incomplete (2)\n EOF\n \n+test_expect_success 'applying a hunk header which overflows fails' '\n+\tcat >patch <<-\\EOF &&\n+\tdiff -u a/file b/file\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -98765432109876543210 +98765432109876543210 @@\n+\t-a\n+\t+b\n+\tEOF\n+\ttest_must_fail git apply patch 2>err &&\n+\techo \"error: corrupt patch at line 4\" >expect &&\n+\ttest_cmp expect err\n+'\n test_done\n\nbase-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05\n-- \ngitgitgadget\n"},{"id":"511528","messageId":"xmqqh65gaqur.fsf@gitster.g","threadId":"62874","inReplyTo":"pull.1858.git.1738235310815.gitgitgadget@gmail.com","subject":"Re: [PATCH] apply: detect overflow when parsing hunk header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-30T22:17:48Z","receivedAt":"2025-01-30T22:17:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> \"git apply\" uses strtoul() to parse the numbers in the hunk header but\n> silently ignores overflows. As LONG_MAX is a legitimate return value for\n> strtoul() we need to set errno to zero before the call to strtoul() and\n> check that it is still zero afterwards. The error message we display is\n> not particularly helpful as it does not say what was wrong.  However, it\n> seems pretty unlikely that users are going to trigger this error in\n> practice and we can always improve it later if needed.\n\nThanks.  We made an effort to use a type that is a bit wider than\n\"int\", but we apparently ignored that the Git userbase will become a\nlot wider some day and unfriendly and/or hostile folks would start\nfeeding malicious input to us X-<.  The check presented here look\ngood, and the fact that there was only one change needed shows how\nwell designed the base code was ;-)\n\nWill queue.  Thanks.\n\n> @@ -1423,7 +1423,10 @@ static int parse_num(const char *line, unsigned long *p)\n>  \n>  \tif (!isdigit(*line))\n>  \t\treturn 0;\n> +\terrno = 0;\n>  \t*p = strtoul(line, &ptr, 10);\n> +\tif (errno)\n> +\t\treturn 0;\n>  \treturn ptr - line;\n>  }\n>  \n> diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh\n> index 146e73d8f55..a5664f3eb3c 100755\n> --- a/t/t4100-apply-stat.sh\n> +++ b/t/t4100-apply-stat.sh\n> @@ -38,4 +38,17 @@ incomplete (1)\n>  incomplete (2)\n>  EOF\n>  \n> +test_expect_success 'applying a hunk header which overflows fails' '\n> +\tcat >patch <<-\\EOF &&\n> +\tdiff -u a/file b/file\n> +\t--- a/file\n> +\t+++ b/file\n> +\t@@ -98765432109876543210 +98765432109876543210 @@\n> +\t-a\n> +\t+b\n> +\tEOF\n> +\ttest_must_fail git apply patch 2>err &&\n> +\techo \"error: corrupt patch at line 4\" >expect &&\n> +\ttest_cmp expect err\n> +'\n>  test_done\n>\n> base-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05\n"}]}