From 7add441984063d2c34fa8de252b8ceb803e7981a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 8 Dec 2014 00:48:13 -0500 Subject: [PATCH 1/2] fsck: properly bound "invalid tag name" error message When we detect an invalid tag-name header in a tag object, like, "tag foo bar\n", we feed the pointer starting at "foo bar" to a printf "%s" formatter. This shows the name, as we want, but then it keeps printing the rest of the tag buffer, rather than stopping at the end of the line. Our tests did not notice because they look only for the matching line, but the bug is that we print much more than we wanted to. So we also adjust the test to be more exact. Note that when fscking tags with "index-pack --strict", this is even worse. index-pack does not add a trailing NUL-terminator after the object, so we may actually read past the buffer and print uninitialized memory. Running t5302 with valgrind does notice the bug for that reason. Signed-off-by: Jeff King Acked-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- fsck.c | 3 ++- t/t1450-fsck.sh | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/fsck.c b/fsck.c index 2fffa434a..88c92e82d 100644 --- a/fsck.c +++ b/fsck.c @@ -423,7 +423,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data, } strbuf_addf(&sb, "refs/tags/%.*s", (int)(eol - buffer), buffer); if (check_refname_format(sb.buf, 0)) - error_func(&tag->object, FSCK_WARN, "invalid 'tag' name: %s", buffer); + error_func(&tag->object, FSCK_WARN, "invalid 'tag' name: %.*s", + (int)(eol - buffer), buffer); buffer = eol + 1; if (!skip_prefix(buffer, "tagger ", &buffer)) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 1b96b4045..785060778 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -209,8 +209,12 @@ test_expect_success 'tag with incorrect tag name & missing tagger' ' echo $tag >.git/refs/tags/wrong && test_when_finished "git update-ref -d refs/tags/wrong" && git fsck --tags 2>out && - grep "invalid .tag. name" out && - grep "expected .tagger. line" out + + cat >expect <<-EOF && + warning in tag $tag: invalid '\''tag'\'' name: wrong name format + warning in tag $tag: invalid format - expected '\''tagger'\'' line + EOF + test_cmp expect out ' test_expect_success 'cleaned up' ' From a1e920a0a7747f0820e62b22b67fd36fb1d74607 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 8 Dec 2014 15:17:55 +0100 Subject: [PATCH 2/2] index-pack: terminate object buffers with NUL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have some tricky checks in fsck that rely on a side effect of require_end_of_header(), and would otherwise easily run outside non-NUL-terminated buffers. This is a bit brittle, so let's make sure that only NUL-terminated buffers are passed around to begin with. Jeff "Peff" King contributed the detailed analysis which call paths are involved and pointed out that we also have to patch the get_data() function in unpack-objects.c, which is what Johannes "Dscho" Schindelin implemented. Signed-off-by: Nguyễn Thái Ngọc Duy Analyzed-by: Jeff King Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 4 ++-- builtin/unpack-objects.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index f2465ff18..f79b04e2c 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -438,7 +438,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size, if (type == OBJ_BLOB && size > big_file_threshold) buf = fixed_buf; else - buf = xmalloc(size); + buf = xmallocz(size); memset(&stream, 0, sizeof(stream)); git_inflate_init(&stream); @@ -543,7 +543,7 @@ static void *unpack_data(struct object_entry *obj, git_zstream stream; int status; - data = xmalloc(consume ? 64*1024 : obj->size); + data = xmallocz(consume ? 64*1024 : obj->size); inbuf = xmalloc((len < 64*1024) ? len : 64*1024); memset(&stream, 0, sizeof(stream)); diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 855d94b90..ac6667242 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -91,7 +91,7 @@ static void use(int bytes) static void *get_data(unsigned long size) { git_zstream stream; - void *buf = xmalloc(size); + void *buf = xmallocz(size); memset(&stream, 0, sizeof(stream));