-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
apply: detect overflow when parsing hunk header #1858
base: master
Are you sure you want to change the base?
apply: detect overflow when parsing hunk header #1858
Conversation
"git apply" uses strtoul() to parse the numbers in the hunk header but silently ignores overflows. As LONG_MAX is a legitimate return value for strtoul() we need to set errno to zero before the call to strtoul() and check that it is still zero afterwards. The error message we display is not particularly helpful as it does not say what was wrong. However, it seems pretty unlikely that users are going to trigger this error in practice and we can always improve it later if needed. Signed-off-by: Phillip Wood <[email protected]>
/submit |
Submitted as [email protected] To fetch this version into
To fetch this version to local tag
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> "git apply" uses strtoul() to parse the numbers in the hunk header but
> silently ignores overflows. As LONG_MAX is a legitimate return value for
> strtoul() we need to set errno to zero before the call to strtoul() and
> check that it is still zero afterwards. The error message we display is
> not particularly helpful as it does not say what was wrong. However, it
> seems pretty unlikely that users are going to trigger this error in
> practice and we can always improve it later if needed.
Thanks. We made an effort to use a type that is a bit wider than
"int", but we apparently ignored that the Git userbase will become a
lot wider some day and unfriendly and/or hostile folks would start
feeding malicious input to us X-<. The check presented here look
good, and the fact that there was only one change needed shows how
well designed the base code was ;-)
Will queue. Thanks.
> @@ -1423,7 +1423,10 @@ static int parse_num(const char *line, unsigned long *p)
>
> if (!isdigit(*line))
> return 0;
> + errno = 0;
> *p = strtoul(line, &ptr, 10);
> + if (errno)
> + return 0;
> return ptr - line;
> }
>
> diff --git a/t/t4100-apply-stat.sh b/t/t4100-apply-stat.sh
> index 146e73d8f55..a5664f3eb3c 100755
> --- a/t/t4100-apply-stat.sh
> +++ b/t/t4100-apply-stat.sh
> @@ -38,4 +38,17 @@ incomplete (1)
> incomplete (2)
> EOF
>
> +test_expect_success 'applying a hunk header which overflows fails' '
> + cat >patch <<-\EOF &&
> + diff -u a/file b/file
> + --- a/file
> + +++ b/file
> + @@ -98765432109876543210 +98765432109876543210 @@
> + -a
> + +b
> + EOF
> + test_must_fail git apply patch 2>err &&
> + echo "error: corrupt patch at line 4" >expect &&
> + test_cmp expect err
> +'
> test_done
>
> base-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05 |
This patch series was integrated into seen via git@c5f2e07. |
This branch is now known as |
This patch series was integrated into seen via git@fcae86a. |
This patch series was integrated into seen via git@2625e6e. |
This patch series was integrated into seen via git@89e5af8. |
There was a status update in the "New Topics" section about the branch "git apply" internally uses unsigned long for line numbers and uses strtoul() to parse numbers on the hunk headers. It however forgot to check parse errors. Will merge to 'next'. source: <[email protected]> |
This patch series was integrated into seen via git@05eeccf. |
This patch series was integrated into next via git@e2b37c2. |
This patch series was integrated into seen via git@1a1ecf6. |
There was a status update in the "Cooking" section about the branch "git apply" internally uses unsigned long for line numbers and uses strtoul() to parse numbers on the hunk headers. It however forgot to check parse errors. Will merge to 'master'. source: <[email protected]> |
This patch series was integrated into seen via git@e38c730. |
This patch series was integrated into seen via git@4bfc672. |
We should do something similar in "git add -p" but I'll wait to see what happens with https://lore.kernel.org/git/[email protected]/ first
Cc: Sören Krecker [email protected]