Re: [PATCH] apply: detect overflow when parsing hunk header

[Junio C Hamano <gitster@pobox.com>]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> "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