From 9b81fae3f9084efa6028b6b45f11b4e1570422ec Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 9 Jan 2025 07:21:30 +0100 Subject: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev` 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=` 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 Signed-off-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin --- builtin/blame.c | 9 ++++++++- t/t8002-blame.sh | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 867032e4c16878..5b497683506631 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -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) diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh index 0147de304b4d10..71fa70a6467923 100755 --- a/t/t8002-blame.sh +++ b/t/t8002-blame.sh @@ -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 '