git-for-windows/git
1
0
Fork 0
mirror of https://github.com/git-for-windows/git.git synced 2026-05-31 02:17:14 -05:00
Code Issues Packages Projects Releases 335 Wiki Activity

builtin/blame: fix out-of-bounds read with excessive --abbrev

Browse Source 
In 6411a0a896 (builtin/blame: fix type of `length` variable when
emitting object ID, 2024-12-06) we have fixed the type of the `length`
variable. In order to avoid a cast from `size_t` to `int` in the call to
printf(3p) with the "%.*s" formatter we have converted the code to
instead use fwrite(3p), which accepts the length as a `size_t`.

It was reported though that this makes us read over the end of the OID
array when the provided `--abbrev=` length exceeds the length of the
object ID. This is because fwrite(3p) of course doesn't stop when it
sees a NUL byte, where as printf(3p) does.

Fix the bug by reverting back to printf(3p) and culling the provided
length to keep it from overflowing when cast to an `int`.

Note that when calling `git blame --abbrev=<N>` with an `N` that is
larger than the maximal OID hex size, there is a subtle side effect that
makes it behave _differently_ than specifying said maximal hex size:
When the command outputs boundary, unblamable or ignored commits' OIDs,
those outputs are prefixed with characters indicating this, and the
`abbrev` value is used to align the information that comes after the
OID, clipping it as needed. Specifying a "too large" abbrev value here
tells Git that yes, we want the full OIDs and don't you worry about
alignment.

Thanks to SHA-256 being _larger_ than the default SHA-1-based OIDs, and
thanks to clipping at `GIT_MAX_HEXSZ`, this change of behavior can only
be observed when running the test in SHA-256 mode.

Nevertheless, this means that we cannot cull at `GIT_MAX_HEXSZ` but at a
slightly larger threshold value.

This patch is a slightly modified version of the one sent as
https://lore.kernel.org/git/20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im
to the Git mailing list.

Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This commit is contained in:
Patrick Steinhardt
2025-01-09 07:21:30 +01:00
committed by Johannes Schindelin
parent 783fac4a95
commit 9b81fae3f9
2 changed files with 12 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
builtin/blame.c
View File

@@ -476,6 +476,13 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?
the_hash_algo->hexsz : (size_t) abbrev;
/*
* Leave enough space for ^, * and ? indicators (boundary,
* unblamable, ignored).
*/
if (length > GIT_MAX_HEXSZ + 3)
length = GIT_MAX_HEXSZ + 3;
if (opt & OUTPUT_COLOR_LINE) {
if (cnt > 0) {
color = repeated_meta_color;
@@ -505,7 +512,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
length--;
putchar('?');
}
fwrite(hex, 1, length, stdout);
printf("%.*s", (int)length, hex);
if (opt & OUTPUT_ANNOTATE_COMPAT) {
const char *name;
if (opt & OUTPUT_SHOW_EMAIL)

4
t/t8002-blame.sh
View File

@@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '
check_abbrev $hexsz --no-abbrev
'
test_expect_success 'blame --abbrev gets truncated' '
check_abbrev 9000 --abbrev=9000 HEAD..
'
test_expect_success '--exclude-promisor-objects does not BUG-crash' '
test_must_fail git blame --exclude-promisor-objects one
'