Skip to content

Commit e7fb2ca

Browse files
pks-tgitster
authored andcommitted
builtin/blame: fix out-of-bounds write with blank boundary commits
When passing the `-b` flag to git-blame(1), then any blamed boundary commits which were marked as uninteresting will not get their actual commit ID printed, but will instead be replaced by a couple of spaces. The flag can lead to an out-of-bounds write as though when combined with `--abbrev=` when the abbreviation length is longer than `GIT_MAX_HEXSZ` as we simply use memset(3p) on that array with the user-provided length directly. The result is most likely that we segfault. An obvious fix would be to cull `length` to `GIT_MAX_HEXSZ` many bytes. But when the underlying object ID is SHA1, and if the abbreviated length exceeds the SHA1 length, it would cause us to print more bytes than desired, and the result would be misaligned. Instead, fix the bug by computing the length via strlen(3p). This makes us write as many bytes as the formatted object ID requires and thus effectively limits the length of what we may end up printing to the length of its hash. If `--abbrev=` asks us to abbreviate to something shorter than the full length of the underlying hash function it would be handled by the call to printf(3p) correctly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 1fbb8d7 commit e7fb2ca

File tree

2 files changed

+21
-3
lines changed

2 files changed

+21
-3
lines changed

builtin/blame.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -489,9 +489,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
489489
fputs(color, stdout);
490490

491491
if (suspect->commit->object.flags & UNINTERESTING) {
492-
if (blank_boundary)
493-
memset(hex, ' ', length);
494-
else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
492+
if (blank_boundary) {
493+
memset(hex, ' ', strlen(hex));
494+
} else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
495495
length--;
496496
putchar('^');
497497
}

t/t8002-blame.sh

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,24 @@ test_expect_success 'blame --abbrev gets truncated with boundary commit' '
134134
check_abbrev $hexsz --abbrev=9000 ^HEAD
135135
'
136136

137+
test_expect_success 'blame --abbrev -b truncates the blank boundary' '
138+
# Note that `--abbrev=` always gets incremented by 1, which is why we
139+
# expect 11 leading spaces and not 10.
140+
cat >expect <<-EOF &&
141+
$(printf "%0.s " $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
142+
EOF
143+
git blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&
144+
test_cmp expect actual
145+
'
146+
147+
test_expect_success 'blame with excessive --abbrev and -b culls to hash length' '
148+
cat >expect <<-EOF &&
149+
$(printf "%0.s " $(test_seq $hexsz)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
150+
EOF
151+
git blame -b --abbrev=9000 ^HEAD -- abbrev.t >actual &&
152+
test_cmp expect actual
153+
'
154+
137155
test_expect_success '--exclude-promisor-objects does not BUG-crash' '
138156
test_must_fail git blame --exclude-promisor-objects one
139157
'

0 commit comments

Comments
 (0)