[PATCH 0/3] hash-object crash fix and new doc and tests

[PATCH 0/3] hash-object crash fix and new doc and tests

[Eric Sunshine]

This patch series fixes a buffer overrun triggered by an extra-long
bogus object type specified via hash-object --literally.

It also adds documentation and tests which were missing from the initial
series which introduced "hash-object --literally".

Eric Sunshine (3):
  git-hash-object.txt: document --literally option
  t1007: add hash-object --literally tests
  write_sha1_file_prepare: fix buffer overrun with extra-long object
    type

 Documentation/git-hash-object.txt | 10 ++++++++--
 sha1_file.c                       | 21 ++++++++++-----------
 t/t1007-hash-object.sh            | 11 +++++++++++
 3 files changed, 29 insertions(+), 13 deletions(-)

-- 
2.4.0.319.g7a04823

[PATCH 1/3] git-hash-object.txt: document --literally option

[Eric Sunshine]

Document the git-hash-object --literally option added by 5ba9a93
(hash-object: add --literally option, 2014-09-11).

While here, also correct a minor typesetting oversight.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

I used Junio's commit message from 5ba9a93 as the basis of the
documentation for --literally and then expanded upon it.

 Documentation/git-hash-object.txt | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt
index 02c1f12..0c75f3b 100644
--- a/Documentation/git-hash-object.txt
+++ b/Documentation/git-hash-object.txt
@@ -9,7 +9,7 @@ git-hash-object - Compute object ID and optionally creates a blob from a file
 SYNOPSIS
 --------
 [verse]
-'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin] [--] <file>...
+'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin [--literally]] [--] <file>...
 'git hash-object' [-t <type>] [-w] --stdin-paths [--no-filters] < <list-of-paths>
 
 DESCRIPTION
@@ -51,7 +51,13 @@ OPTIONS
 	Hash the contents as is, ignoring any input filter that would
 	have been chosen by the attributes mechanism, including the end-of-line
 	conversion. If the file is read from standard input then this
-	is always implied, unless the --path option is given.
+	is always implied, unless the `--path` option is given.
+
+--literally::
+	Allow `--stdin` to hash any garbage into a loose object which might not
+	otherwise pass standard object parsing or git-fsck checks. Useful for
+	stress-testing Git itself or reproducing characteristics of corrupt or
+	bogus objects encountered in the wild.
 
 GIT
 ---
-- 
2.4.0.319.g7a04823

[PATCH 2/3] t1007: add hash-object --literally tests

[Eric Sunshine]

git-hash-object learned a --literally option in 5ba9a93 (hash-object:
add --literally option, 2014-09-11). Check that --literally allows
object creation with a bogus type.

Also add a failing test demonstrating a crash (buffer overflow leading
to stack corruption) when the bogus type is lengthy.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Although the crash seems to have manifested[1] only on Mac OS X when
testing Karthik's "cat-file --allow-unknown-type" series, I made the
bogus object type extremely long in the failing test included here in
order to ensure that it manifests (hopefully) everywhere. At this
length, the crash manifest on Linux as well.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/268262/focus=268304

 t/t1007-hash-object.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index f83df8e..0e65577 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -201,4 +201,15 @@ test_expect_success 'corrupt tag' '
 	test_must_fail git hash-object -t tag --stdin </dev/null
 '
 
+test_expect_success '--literally' '
+	t=1234567890 &&
+	echo example | git hash-object -t $t --literally --stdin
+'
+
+test_expect_failure '--literally with extra-long type' '
+	t=12345678901234567890123456789012345678901234567890 &&
+	t="$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t" &&
+	echo example | git hash-object -t $t --literally --stdin
+'
+
 test_done
-- 
2.4.0.319.g7a04823

[PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Eric Sunshine]

git-hash-object learned --literally in 5ba9a93 (hash-object: add
--literally option, 2014-09-11) which can be used to craft a
corrupt/broken object of unknown type. When the user-provided type is
particularly long, it can overflow the relatively small stack-based
character array handed to write_sha1_file_prepare() by hash_sha1_file()
and write_sha1_file(), leading to stack corruption (and crash).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

The composed 'hdr' is supposed to be NUL-terminated, and code which
accesses 'hdr' expects the NUL to be included as part of its length.
Although strbuf ensures that a NUL byte follows the string content, I
took the precaution of explicitly adding NUL when formulating 'hdr'

    strbuf_addf(hdr, "%s %lu%c", type, len, '\0');

so that callers can just say hdr.len when the length is needed, rather
than having to remember to say hdr.len+1.


I haven't fully convinced myself that this fix is appropriate since it
penalizes _all_ callers of hash_sha1_file() and write_sha1_file() with
an extra heap allocation (via strbuf), even though "hash-object
--literally" is the only mechanism by which an overly-long object type
can arrive.

 sha1_file.c            | 21 ++++++++++-----------
 t/t1007-hash-object.sh |  2 +-
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 88f06ba..6d3fa26 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2795,16 +2795,16 @@ void *read_object_with_reference(const unsigned char *sha1,
 
 static void write_sha1_file_prepare(const void *buf, unsigned long len,
                                     const char *type, unsigned char *sha1,
-                                    char *hdr, int *hdrlen)
+				    struct strbuf *hdr)
 {
 	git_SHA_CTX c;
 
 	/* Generate the header */
-	*hdrlen = sprintf(hdr, "%s %lu", type, len)+1;
+	strbuf_addf(hdr, "%s %lu%c", type, len, '\0');
 
 	/* Sha1.. */
 	git_SHA1_Init(&c);
-	git_SHA1_Update(&c, hdr, *hdrlen);
+	git_SHA1_Update(&c, hdr->buf, hdr->len);
 	git_SHA1_Update(&c, buf, len);
 	git_SHA1_Final(sha1, &c);
 }
@@ -2865,9 +2865,8 @@ static int write_buffer(int fd, const void *buf, size_t len)
 int hash_sha1_file(const void *buf, unsigned long len, const char *type,
                    unsigned char *sha1)
 {
-	char hdr[32];
-	int hdrlen;
-	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
+	struct strbuf hdr = STRBUF_INIT;
+	write_sha1_file_prepare(buf, len, type, sha1, &hdr);
 	return 0;
 }
 
@@ -3005,18 +3004,18 @@ static int freshen_packed_object(const unsigned char *sha1)
 int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
 {
 	unsigned char sha1[20];
-	char hdr[32];
-	int hdrlen;
+	struct strbuf hdr = STRBUF_INIT;
 
-	/* Normally if we have it in the pack then we do not bother writing
+	/*
+	 * Normally if we have it in the pack then we do not bother writing
 	 * it out into .git/objects/??/?{38} file.
 	 */
-	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
+	write_sha1_file_prepare(buf, len, type, sha1, &hdr);
 	if (returnsha1)
 		hashcpy(returnsha1, sha1);
 	if (freshen_loose_object(sha1) || freshen_packed_object(sha1))
 		return 0;
-	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
+	return write_loose_object(sha1, hdr.buf, hdr.len, buf, len, 0);
 }
 
 int force_object_loose(const unsigned char *sha1, time_t mtime)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 0e65577..7c3dcfb 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -206,7 +206,7 @@ test_expect_success '--literally' '
 	echo example | git hash-object -t $t --literally --stdin
 '
 
-test_expect_failure '--literally with extra-long type' '
+test_expect_success '--literally with extra-long type' '
 	t=12345678901234567890123456789012345678901234567890 &&
 	t="$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t" &&
 	echo example | git hash-object -t $t --literally --stdin
-- 
2.4.0.319.g7a04823

Re: [PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> git-hash-object learned --literally in 5ba9a93 (hash-object: add
> --literally option, 2014-09-11) which can be used to craft a
> corrupt/broken object of unknown type. When the user-provided type is
> particularly long, it can overflow the relatively small stack-based
> character array handed to write_sha1_file_prepare() by hash_sha1_file()
> and write_sha1_file(), leading to stack corruption (and crash).
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Thanks.

> ---
>
> The composed 'hdr' is supposed to be NUL-terminated, and code which
> accesses 'hdr' expects the NUL to be included as part of its length.
> Although strbuf ensures that a NUL byte follows the string content, I
> took the precaution of explicitly adding NUL when formulating 'hdr'
>
>     strbuf_addf(hdr, "%s %lu%c", type, len, '\0');
>
> so that callers can just say hdr.len when the length is needed, rather
> than having to remember to say hdr.len+1.

That is unnecessary and may turn out to be more confusing than it is
worth in the long run; though I do not think it matters too much.

> I haven't fully convinced myself that this fix is appropriate since it
> penalizes _all_ callers of hash_sha1_file() and write_sha1_file() with
> an extra heap allocation (via strbuf), even though "hash-object
> --literally" is the only mechanism by which an overly-long object type
> can arrive.

I am moderately unhappy about fixing it this way for the exact
reason you stated aboe.

write_sha1_file_prepare() can stay the same as before, and we can
use a variant of hash_sha1_file() only when doing the --literally
thing to use a special allocation.  Perhaps like this?

The patch was done on top of yours; it reverts the change to
write_sha1_file_prepare() and its callers, and instead adds a
separate helper.  We may want to restructure the latter half of
write_sha1_file() that does the freshen-or-write into a helper
function to share more logic, though.

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 207b90c..ef383e1 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne
 
 	if (strbuf_read(&buf, fd, 4096) < 0)
 		ret = -1;
-	else if (flags & HASH_WRITE_OBJECT)
-		ret = write_sha1_file(buf.buf, buf.len, type, sha1);
 	else
-		ret = hash_sha1_file(buf.buf, buf.len, type, sha1);
+		ret = hash_sha1_file_literally(&buf, type, sha1, flags);
 	strbuf_release(&buf);
 	return ret;
 }
diff --git a/cache.h b/cache.h
index 3d3244b..a37423e 100644
--- a/cache.h
+++ b/cache.h
@@ -874,6 +874,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned
 extern int sha1_object_info(const unsigned char *, unsigned long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
+extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_noatime(const char *name);
diff --git a/sha1_file.c b/sha1_file.c
index 6d3fa26..c8ab069 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2795,16 +2795,16 @@ void *read_object_with_reference(const unsigned char *sha1,
 
 static void write_sha1_file_prepare(const void *buf, unsigned long len,
                                     const char *type, unsigned char *sha1,
-				    struct strbuf *hdr)
+				    char *hdr, int *hdrlen)
 {
 	git_SHA_CTX c;
 
 	/* Generate the header */
-	strbuf_addf(hdr, "%s %lu%c", type, len, '\0');
+	*hdrlen = sprintf(hdr, "%s %lu", type, len)+1;
 
 	/* Sha1.. */
 	git_SHA1_Init(&c);
-	git_SHA1_Update(&c, hdr->buf, hdr->len);
+	git_SHA1_Update(&c, hdr, *hdrlen);
 	git_SHA1_Update(&c, buf, len);
 	git_SHA1_Final(sha1, &c);
 }
@@ -2865,8 +2865,9 @@ static int write_buffer(int fd, const void *buf, size_t len)
 int hash_sha1_file(const void *buf, unsigned long len, const char *type,
                    unsigned char *sha1)
 {
-	struct strbuf hdr = STRBUF_INIT;
-	write_sha1_file_prepare(buf, len, type, sha1, &hdr);
+	char hdr[32];
+	int hdrlen;
+	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
 	return 0;
 }
 
@@ -3004,18 +3005,43 @@ static int freshen_packed_object(const unsigned char *sha1)
 int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
 {
 	unsigned char sha1[20];
-	struct strbuf hdr = STRBUF_INIT;
+	char hdr[32];
+	int hdrlen;
 
-	/*
-	 * Normally if we have it in the pack then we do not bother writing
+	/* Normally if we have it in the pack then we do not bother writing
 	 * it out into .git/objects/??/?{38} file.
 	 */
-	write_sha1_file_prepare(buf, len, type, sha1, &hdr);
+	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
 	if (returnsha1)
 		hashcpy(returnsha1, sha1);
 	if (freshen_loose_object(sha1) || freshen_packed_object(sha1))
 		return 0;
-	return write_loose_object(sha1, hdr.buf, hdr.len, buf, len, 0);
+	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
+}
+
+int hash_sha1_file_literally(struct strbuf *buf, const char *type,
+			     unsigned char *sha1, unsigned flags)
+{
+	struct strbuf header = STRBUF_INIT;
+	int hdrlen, status = 0;
+
+	/* type string, SP, %lu of the length plus NUL must fit this */
+	strbuf_grow(&header, strlen(type) + 20);
+
+	write_sha1_file_prepare(buf->buf, buf->len, type, sha1,
+				header.buf, &hdrlen);
+
+	if (!(flags & HASH_WRITE_OBJECT))
+		goto cleanup;
+
+	if (freshen_loose_object(sha1) || freshen_packed_object(sha1))
+		goto cleanup;
+	status = write_loose_object(sha1, header.buf, hdrlen,
+				    buf->buf, buf->len, 0);
+
+cleanup:
+	strbuf_release(&header);
+	return status;
 }
 
 int force_object_loose(const unsigned char *sha1, time_t mtime)

Re: [PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Junio C Hamano]

By the way, you would notice that "if we have returnsha1, then copy
that in" bit is not in the new literally codepath but still is in
write_sha1_file().

I do not think any caller passes a NULL as return_sha1 in today's
code, which made me curious.

It turns out to be a remnant of d6d3f9d0 (This implements the new
"recursive tree" write-tree., 2005-04-09); before that change,
write_sha1_file() did not have an ability to tell the caller what
object it wrote, and Linus made it optional for the callers when he
added the return_sha1[] out parameter, but all of its callers did
want the resulting object name.

So I think it is safe and sensible to do the following change
regardless of "hash-object --literally" fix.

 sha1_file.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index c8ab069..96e813f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3002,9 +3002,8 @@ static int freshen_packed_object(const unsigned char *sha1)
 	return find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);
 }
 
-int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
+int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1)
 {
-	unsigned char sha1[20];
 	char hdr[32];
 	int hdrlen;
 
@@ -3012,8 +3011,6 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
 	 * it out into .git/objects/??/?{38} file.
 	 */
 	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
-	if (returnsha1)
-		hashcpy(returnsha1, sha1);
 	if (freshen_loose_object(sha1) || freshen_packed_object(sha1))
 		return 0;
 	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);

[PATCH 0/4] "hash-object --literally" fixes

[Junio C Hamano]

This is a rework of Eric's fix (Thanks!), to avoid using strbuf all
the time, even when we are not doing "hash-object --literally".

Eric Sunshine (3):
  git-hash-object.txt: document --literally option
  write_sha1_file_prepare: fix buffer overrun with extra-long object type
  t1007: add hash-object --literally tests

Junio C Hamano (1):
  write_sha1_file(): do not use a separate sha1[] array

 Documentation/git-hash-object.txt | 10 ++++++++--
 builtin/hash-object.c             |  4 +---
 cache.h                           |  1 +
 sha1_file.c                       | 32 +++++++++++++++++++++++++++-----
 t/t1007-hash-object.sh            | 11 +++++++++++
 5 files changed, 48 insertions(+), 10 deletions(-)

-- 
2.4.0-302-g6743426

[PATCH 1/4] git-hash-object.txt: document --literally option

[Junio C Hamano]

From: Eric Sunshine <sunshine@sunshineco.com>

Document the git-hash-object --literally option added by 5ba9a93
(hash-object: add --literally option, 2014-09-11).

While here, also correct a minor typesetting oversight.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * As posted by Eric

 Documentation/git-hash-object.txt | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt
index 02c1f12..0c75f3b 100644
--- a/Documentation/git-hash-object.txt
+++ b/Documentation/git-hash-object.txt
@@ -9,7 +9,7 @@ git-hash-object - Compute object ID and optionally creates a blob from a file
 SYNOPSIS
 --------
 [verse]
-'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin] [--] <file>...
+'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin [--literally]] [--] <file>...
 'git hash-object' [-t <type>] [-w] --stdin-paths [--no-filters] < <list-of-paths>
 
 DESCRIPTION
@@ -51,7 +51,13 @@ OPTIONS
 	Hash the contents as is, ignoring any input filter that would
 	have been chosen by the attributes mechanism, including the end-of-line
 	conversion. If the file is read from standard input then this
-	is always implied, unless the --path option is given.
+	is always implied, unless the `--path` option is given.
+
+--literally::
+	Allow `--stdin` to hash any garbage into a loose object which might not
+	otherwise pass standard object parsing or git-fsck checks. Useful for
+	stress-testing Git itself or reproducing characteristics of corrupt or
+	bogus objects encountered in the wild.
 
 GIT
 ---
-- 
2.4.0-302-g6743426

[PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Junio C Hamano]

From: Eric Sunshine <sunshine@sunshineco.com>

git-hash-object learned --literally in 5ba9a93 (hash-object: add
--literally option, 2014-09-11) which can be used to craft a
corrupt/broken object of unknown type.

When the user-provided type is particularly long, however, it can
overflow the relatively small stack-based character array handed to
write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),
leading to stack corruption (and crash).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Tweaked and backported directly on top of 5ba9a93b (hash-object:
   add --literally option, 2014-09-11) which is v2.2.0-rc0~88^2

 builtin/hash-object.c |  4 +---
 cache.h               |  1 +
 sha1_file.c           | 27 ++++++++++++++++++++++++++-
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 6158363..887a8ea 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne
 
 	if (strbuf_read(&buf, fd, 4096) < 0)
 		ret = -1;
-	else if (flags & HASH_WRITE_OBJECT)
-		ret = write_sha1_file(buf.buf, buf.len, type, sha1);
 	else
-		ret = hash_sha1_file(buf.buf, buf.len, type, sha1);
+		ret = hash_sha1_file_literally(&buf, type, sha1, flags);
 	strbuf_release(&buf);
 	return ret;
 }
diff --git a/cache.h b/cache.h
index dfa1a56..2da7740 100644
--- a/cache.h
+++ b/cache.h
@@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned
 extern int sha1_object_info(const unsigned char *, unsigned long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
+extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_noatime(const char *name);
diff --git a/sha1_file.c b/sha1_file.c
index c08c0cb..0fe3f29 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2962,6 +2962,31 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
 	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
 }
 
+int hash_sha1_file_literally(struct strbuf *buf, const char *type,
+			     unsigned char *sha1, unsigned flags)
+{
+	struct strbuf header = STRBUF_INIT;
+	int hdrlen, status = 0;
+
+	/* type string, SP, %lu of the length plus NUL must fit this */
+	strbuf_grow(&header, strlen(type) + 20);
+
+	write_sha1_file_prepare(buf->buf, buf->len, type, sha1,
+				header.buf, &hdrlen);
+
+	if (!(flags & HASH_WRITE_OBJECT))
+		goto cleanup;
+
+	if (has_sha1_file(sha1))
+		goto cleanup;
+	status = write_loose_object(sha1, header.buf, hdrlen,
+				    buf->buf, buf->len, 0);
+
+cleanup:
+	strbuf_release(&header);
+	return status;
+}
+
 int force_object_loose(const unsigned char *sha1, time_t mtime)
 {
 	void *buf;
-- 
2.4.0-302-g6743426

Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Eric Sunshine]

On Mon, May 4, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>

Thanks for re-rerolling this series. Considering that the only bits
left from me are the diagnosis and the (mostly intact) commit message,
perhaps the authorship should be changed, or at the very least a big
"Helped-by: Junio" added? Anyhow, a few minor comments below...

> write_sha1_file_prepare: fix buffer overrun with extra-long object type

Although the overrun happened in write_sha1_file_prepare(), that
function is no longer the focus of the patch. Would make sense to
rephrase the subject more generally as:

    sha1_file: fix buffer overrun with extra-long object type

or something.

More below.

> git-hash-object learned --literally in 5ba9a93 (hash-object: add
> --literally option, 2014-09-11) which can be used to craft a
> corrupt/broken object of unknown type.
>
> When the user-provided type is particularly long, however, it can
> overflow the relatively small stack-based character array handed to
> write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),
> leading to stack corruption (and crash).
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/cache.h b/cache.h
> index dfa1a56..2da7740 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned
>  extern int sha1_object_info(const unsigned char *, unsigned long *);
>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
>  extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
> +extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);

A few questions:

What's the value of making the first argument of
hash_sha1_file_literally() a strbuf rather than the two-argument buf &
len accepted by hash_sha1_file() and write_sha1_file()? Is the
inconsistency warranted?

Would it make sense to name the third argument "sha1" instead of
"return_sha1" to match the argument name of hash_sha1_file()?

And, as an aside, should your new patch 4/4 rename "return_sha1" to
"sha1" in the write_sha1_file() prototype also?

>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
>  extern int force_object_loose(const unsigned char *sha1, time_t mtime);
>  extern int git_open_noatime(const char *name);
> diff --git a/sha1_file.c b/sha1_file.c
> index c08c0cb..0fe3f29 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2962,6 +2962,31 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
>         return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
>  }
>
> +int hash_sha1_file_literally(struct strbuf *buf, const char *type,
> +                            unsigned char *sha1, unsigned flags)
> +{
> +       struct strbuf header = STRBUF_INIT;
> +       int hdrlen, status = 0;
> +
> +       /* type string, SP, %lu of the length plus NUL must fit this */
> +       strbuf_grow(&header, strlen(type) + 20);

A couple comments:

First, given that the largest 64-bit unsigned long value
(18,446,744,073,709,551,615) is 20 characters, do we want to be really
pedantic and add 22 instead of 20?

Second, is strbuf overkill in this situation when a simple
xmalloc()/free() would do?

> +       write_sha1_file_prepare(buf->buf, buf->len, type, sha1,
> +                               header.buf, &hdrlen);
> +
> +       if (!(flags & HASH_WRITE_OBJECT))
> +               goto cleanup;
> +
> +       if (has_sha1_file(sha1))
> +               goto cleanup;
> +       status = write_loose_object(sha1, header.buf, hdrlen,
> +                                   buf->buf, buf->len, 0);
> +
> +cleanup:
> +       strbuf_release(&header);
> +       return status;
> +}
> +
>  int force_object_loose(const unsigned char *sha1, time_t mtime)
>  {
>         void *buf;
> --
> 2.4.0-302-g6743426

Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +extern int hash_sha1_file_literally(struct strbuf *buf, const char
>> *type, unsigned char *return_sha1, unsigned flags);
>
> A few questions:
>
> What's the value of making the first argument of
> hash_sha1_file_literally() a strbuf rather than the two-argument
> buf & len accepted by hash_sha1_file() and write_sha1_file()? Is
> the inconsistency warranted?

I do not care either way, as this is really meant to be a
single-purpose wrapper for a single caller.

> Would it make sense to name the third argument "sha1" instead of
> "return_sha1" to match the argument name of hash_sha1_file()?
>
> And, as an aside, should your new patch 4/4 rename "return_sha1" to
> "sha1" in the write_sha1_file() prototype also?

Because most of the read-cache.c functions take object name and do
something about it, but these small number of functions compute and
return object name, I actually think it is a good way to keep
"return" in the name to remind those who are writing or reading
callers.

>> +       /* type string, SP, %lu of the length plus NUL must fit this */
>> +       strbuf_grow(&header, strlen(type) + 20);
>
> A couple comments:
>
> First, given that the largest 64-bit unsigned long value
> (18,446,744,073,709,551,615) is 20 characters, do we want to be really
> pedantic and add 22 instead of 20?

32 is fine ;-)

> Second, is strbuf overkill in this situation when a simple
> xmalloc()/free() would do?

I think underneath the number of xmalloc()/free() calls are the
same.  The code that uses strbuf abstraction is easier to read, I
would think.

Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, May 4, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> From: Eric Sunshine <sunshine@sunshineco.com>
>
> Thanks for re-rerolling this series. Considering that the only bits
> left from me are the diagnosis and the (mostly intact) commit message,
> perhaps the authorship should be changed, or at the very least a big
> "Helped-by: Junio" added? Anyhow, a few minor comments below...

I am a bit too lazy to take the ownership, so I decided only to take
the blame ;-)

Here is a replacement; all the other patches stay the same.

-- >8 --
From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mon, 4 May 2015 03:25:15 -0400
Subject: [PATCH] hash-object --literally: fix buffer overrun with extra-long object type

"hash-object" learned in 5ba9a93 (hash-object: add --literally
option, 2014-09-11) to allow crafting a corrupt/broken object of
unknown type.

When the user-provided type is particularly long, however, it can
overflow the relatively small stack-based character array handed to
write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),
leading to stack corruption (and crash).  Introduce a custom helper
to allow arbitrarily long typenames just for "hash-object --literally".

[jc: Eric's original used a strbuf in the more common codepaths, and
I rewrote it to avoid penalizing the non-literally code. Bugs are mine]

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/hash-object.c |  4 +---
 cache.h               |  1 +
 sha1_file.c           | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 6158363..17e8bfdc 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne
 
 	if (strbuf_read(&buf, fd, 4096) < 0)
 		ret = -1;
-	else if (flags & HASH_WRITE_OBJECT)
-		ret = write_sha1_file(buf.buf, buf.len, type, sha1);
 	else
-		ret = hash_sha1_file(buf.buf, buf.len, type, sha1);
+		ret = hash_sha1_file_literally(buf.buf, buf.len, type, sha1, flags);
 	strbuf_release(&buf);
 	return ret;
 }
diff --git a/cache.h b/cache.h
index dfa1a56..e037cad 100644
--- a/cache.h
+++ b/cache.h
@@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned
 extern int sha1_object_info(const unsigned char *, unsigned long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
+extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_noatime(const char *name);
diff --git a/sha1_file.c b/sha1_file.c
index c08c0cb..dc940e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2962,6 +2962,27 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
 	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
 }
 
+int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type,
+			     unsigned char *sha1, unsigned flags)
+{
+	char *header;
+	int hdrlen, status = 0;
+
+	/* type string, SP, %lu of the length plus NUL must fit this */
+	header = xmalloc(strlen(type) + 32);
+	write_sha1_file_prepare(buf, len, type, sha1, header, &hdrlen);
+
+	if (!(flags & HASH_WRITE_OBJECT))
+		goto cleanup;
+	if (has_sha1_file(sha1))
+		goto cleanup;
+	status = write_loose_object(sha1, header, hdrlen, buf, len, 0);
+
+cleanup:
+	free(header);
+	return status;
+}
+
 int force_object_loose(const unsigned char *sha1, time_t mtime)
 {
 	void *buf;
-- 
2.4.0-311-gf1d9b8d

Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

[Eric Sunshine]

On Tue, May 5, 2015 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> Date: Mon, 4 May 2015 03:25:15 -0400
> Subject: [PATCH] hash-object --literally: fix buffer overrun with extra-long object type
>
> "hash-object" learned in 5ba9a93 (hash-object: add --literally
> option, 2014-09-11) to allow crafting a corrupt/broken object of
> unknown type.
>
> When the user-provided type is particularly long, however, it can
> overflow the relatively small stack-based character array handed to
> write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),
> leading to stack corruption (and crash).  Introduce a custom helper
> to allow arbitrarily long typenames just for "hash-object --literally".
>
> [jc: Eric's original used a strbuf in the more common codepaths, and
> I rewrote it to avoid penalizing the non-literally code. Bugs are mine]

Thanks for re-rolling again. The amended Subject: works nicely now,
and the addition to the commit message makes sense. Also, the code
changes in response to the minor questions I raised[1] all look good.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/268306/focus=268374

(rest of patch left unsnipped)

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index 6158363..17e8bfdc 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne
>
>         if (strbuf_read(&buf, fd, 4096) < 0)
>                 ret = -1;
> -       else if (flags & HASH_WRITE_OBJECT)
> -               ret = write_sha1_file(buf.buf, buf.len, type, sha1);
>         else
> -               ret = hash_sha1_file(buf.buf, buf.len, type, sha1);
> +               ret = hash_sha1_file_literally(buf.buf, buf.len, type, sha1, flags);
>         strbuf_release(&buf);
>         return ret;
>  }
> diff --git a/cache.h b/cache.h
> index dfa1a56..e037cad 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned
>  extern int sha1_object_info(const unsigned char *, unsigned long *);
>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
>  extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
> +extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
>  extern int force_object_loose(const unsigned char *sha1, time_t mtime);
>  extern int git_open_noatime(const char *name);
> diff --git a/sha1_file.c b/sha1_file.c
> index c08c0cb..dc940e6 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2962,6 +2962,27 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
>         return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
>  }
>
> +int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type,
> +                            unsigned char *sha1, unsigned flags)
> +{
> +       char *header;
> +       int hdrlen, status = 0;
> +
> +       /* type string, SP, %lu of the length plus NUL must fit this */
> +       header = xmalloc(strlen(type) + 32);
> +       write_sha1_file_prepare(buf, len, type, sha1, header, &hdrlen);
> +
> +       if (!(flags & HASH_WRITE_OBJECT))
> +               goto cleanup;
> +       if (has_sha1_file(sha1))
> +               goto cleanup;
> +       status = write_loose_object(sha1, header, hdrlen, buf, len, 0);
> +
> +cleanup:
> +       free(header);
> +       return status;
> +}
> +
>  int force_object_loose(const unsigned char *sha1, time_t mtime)
>  {
>         void *buf;
> --
> 2.4.0-311-gf1d9b8d

[PATCH 3/4] t1007: add hash-object --literally tests

[Junio C Hamano]

From: Eric Sunshine <sunshine@sunshineco.com>

git-hash-object learned a --literally option in 5ba9a93
(hash-object: add --literally option, 2014-09-11). Check that
--literally allows object creation with a bogus type, with two
type strings whose length is reasonably short and very long.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Almost as posted by Eric, but the fix happens earlier in the
   series so both tests expect to succeed.

 t/t1007-hash-object.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index f83df8e..7c3dcfb 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -201,4 +201,15 @@ test_expect_success 'corrupt tag' '
 	test_must_fail git hash-object -t tag --stdin </dev/null
 '
 
+test_expect_success '--literally' '
+	t=1234567890 &&
+	echo example | git hash-object -t $t --literally --stdin
+'
+
+test_expect_success '--literally with extra-long type' '
+	t=12345678901234567890123456789012345678901234567890 &&
+	t="$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t$t" &&
+	echo example | git hash-object -t $t --literally --stdin
+'
+
 test_done
-- 
2.4.0-302-g6743426

[PATCH 4/4] write_sha1_file(): do not use a separate sha1[] array

[Junio C Hamano]

In the beginning, write_sha1_file() did not have a way to tell the
caller the name of the object it wrote to the caller.  This was
changed in d6d3f9d0 (This implements the new "recursive tree"
write-tree., 2005-04-09) by adding the "returnsha1" parameter to the
function so that the callers who are interested in the value can
optionally pass a pointer to receive it.

It turns out that all callers do want to know the name of the object
it just has written.  Nobody passes a NULL to this parameter, hence
it is not necessary to use a separate sha1[] array to receive the
result from  write_sha1_file_prepare(), and copy the result to the
returnsha1 supplied by the caller.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is something I noticed while in the vicinity.

 sha1_file.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 0fe3f29..cec0ef2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2945,9 +2945,8 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
 	return move_temp_to_file(tmp_file, filename);
 }
 
-int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
+int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1)
 {
-	unsigned char sha1[20];
 	char hdr[32];
 	int hdrlen;
 
@@ -2955,8 +2954,6 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
 	 * it out into .git/objects/??/?{38} file.
 	 */
 	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
-	if (returnsha1)
-		hashcpy(returnsha1, sha1);
 	if (has_sha1_file(sha1))
 		return 0;
 	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
-- 
2.4.0-302-g6743426