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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2025, 14:59 UTC
Message-ID: <xmqqldvkkp8j.fsf@gitster.g>
In-Reply-To: <20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

> 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.

Show 8 quoted lines

>  			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.