Re: [PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`

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

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 867032e4c16878ffd56df8a73162b89ca4bd2694..f92e487bed22eec576a4716f2e654cb61efb9903 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -505,7 +505,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int

Mental note: the "hex" in question that is used later is defined as

	char hex[GIT_MAX_HEXSZ + 1];

at the top of this scope.

>  			length--;
>  			putchar('?');
>  		}
> -		fwrite(hex, 1, length, stdout);
> +
> +		if (length > GIT_MAX_HEXSZ)
> +			length = GIT_MAX_HEXSZ;
> +		printf("%.*s", (int)length, hex);

This is not wrong per-se, but leaves a funny aftertaste after
reading it, especially if the reader knows the original *bug* was
about showing past the end of the string in "hex".  The question is
"what if the current hash function produces shorter than MAX_HEXSZ?"

The updated code is correct only because it reinstates the use of
printf() so the "length" being longer than the string itself no
longer matters, and the only requirement on "length" is not to
read beyond the end of hex[] array.  So feeding the length of the
array as the limit is not wrong, even though it may be feeding a
limit that is larger than the string stored in the array.

An obvious alternative would have been to base the limit on the
value of strlen(hex) and then we may even keep using fwrite(); then
we do not have worry about "what if MAX_HEXSZ is larger than the
current hash function?" and nonsense like that (I personally prefer
what is being reviewed much better; please do not take this "obvious
alternative" as a suggestion).

Limiting the "length" here does improve the resulting code, relative
to v1 iteration.  This printf is about showing the hexstring and we
are making sure we do not read past the end of the array that holds
the string, and doing it here means we do not have to worry about
leading prefix characters about boundary etc.  Very good
improvement.

Thanks.