mbox series

[v3,0/2] builtin/blame: fix out-of-bounds reads and writes

Message ID 20250110-b4-pks-blame-truncate-hash-length-v3-0-e61f25b68f30@pks.im (mailing list archive)
Headers show
Series builtin/blame: fix out-of-bounds reads and writes | expand

Message

Patrick Steinhardt Jan. 10, 2025, 11:26 a.m. UTC 
Hi,

This fixes the issues reported in [1] and [2]. Thanks!

Changes in v2:

  - Take into account that we may strip ^, * and ? indicators by moving
    around the check.
  - Fix the testcase so that it actually fails without the fix.
  - Link to v1: https://lore.kernel.org/r/20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im

Changes in v3:

  - Add another testcase for boundary commits.
  - Fix another out-of-bound write noticed by Coverity. This bug is not
    a regression in v2.48.0, but is a preexisting error.
  - Simplify the printf statement a bit by using a ternary statement.
  - Link to v2: https://lore.kernel.org/r/20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im

Patrick

[1]: <4d812802-afbc-4635-7a19-73896fcda625@gmx.de>
[2]: <48ca0114-124b-e3f5-af80-1e302bf9ce52@gmx.de>

---
Patrick Steinhardt (2):
      builtin/blame: fix out-of-bounds read with excessive `--abbrev`
      builtin/blame: fix out-of-bounds write with blank boundary commits

 builtin/blame.c  |  9 +++++----
 t/t8002-blame.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)

Range-diff versus v2:

1:  7065637d8e ! 1:  3865bfe643 builtin/blame: fix out-of-bounds read with excessive `--abbrev`
    @@ builtin/blame.c: static void emit_other(struct blame_scoreboard *sb, struct blam
      		}
     -		fwrite(hex, 1, length, stdout);
     +
    -+		if (length > GIT_MAX_HEXSZ)
    -+			length = GIT_MAX_HEXSZ;
    -+		printf("%.*s", (int)length, hex);
    ++		printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);
      		if (opt & OUTPUT_ANNOTATE_COMPAT) {
      			const char *name;
      			if (opt & OUTPUT_SHOW_EMAIL)
    @@ t/t8002-blame.sh: test_expect_success '--no-abbrev works like --abbrev with full
     +test_expect_success 'blame --abbrev gets truncated' '
     +	check_abbrev $hexsz --abbrev=9000 HEAD
     +'
    ++
    ++test_expect_success 'blame --abbrev gets truncated with boundary commit' '
    ++	check_abbrev $hexsz --abbrev=9000 ^HEAD
    ++'
     +
      test_expect_success '--exclude-promisor-objects does not BUG-crash' '
      	test_must_fail git blame --exclude-promisor-objects one
-:  ---------- > 2:  af0af67a8a builtin/blame: fix out-of-bounds write with blank boundary commits

---
base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e
change-id: 20250109-b4-pks-blame-truncate-hash-length-c875cac66d71