{"thread":{"id":"39231","subject":"[PATCH 0/3] hash-object crash fix and new doc and tests","startedAt":"2015-05-04T07:25:12Z","lastAt":"2015-05-05T18:49:50Z","messageCount":15,"participants":["Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"260504","messageId":"1430724315-524-1-git-send-email-sunshine@sunshineco.com","threadId":"39231","inReplyTo":null,"subject":"[PATCH 0/3] hash-object crash fix and new doc and tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-04T07:25:12Z","receivedAt":"2015-05-04T07:25:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This patch series fixes a buffer overrun triggered by an extra-long\nbogus object type specified via hash-object --literally.\n\nIt also adds documentation and tests which were missing from the initial\nseries which introduced \"hash-object --literally\".\n\nEric Sunshine (3):\n  git-hash-object.txt: document --literally option\n  t1007: add hash-object --literally tests\n  write_sha1_file_prepare: fix buffer overrun with extra-long object\n    type\n\n Documentation/git-hash-object.txt | 10 ++++++++--\n sha1_file.c                       | 21 ++++++++++-----------\n t/t1007-hash-object.sh            | 11 +++++++++++\n 3 files changed, 29 insertions(+), 13 deletions(-)\n\n-- \n2.4.0.319.g7a04823\n"},{"id":"260505","messageId":"1430724315-524-2-git-send-email-sunshine@sunshineco.com","threadId":"39231","inReplyTo":"1430724315-524-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 1/3] git-hash-object.txt: document --literally option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-04T07:25:13Z","receivedAt":"2015-05-04T07:25:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Document the git-hash-object --literally option added by 5ba9a93\n(hash-object: add --literally option, 2014-09-11).\n\nWhile here, also correct a minor typesetting oversight.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nI used Junio's commit message from 5ba9a93 as the basis of the\ndocumentation for --literally and then expanded upon it.\n\n Documentation/git-hash-object.txt | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt\nindex 02c1f12..0c75f3b 100644\n--- a/Documentation/git-hash-object.txt\n+++ b/Documentation/git-hash-object.txt\n@@ -9,7 +9,7 @@ git-hash-object - Compute object ID and optionally creates a blob from a file\n SYNOPSIS\n --------\n [verse]\n-'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin] [--] <file>...\n+'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin [--literally]] [--] <file>...\n 'git hash-object' [-t <type>] [-w] --stdin-paths [--no-filters] < <list-of-paths>\n \n DESCRIPTION\n@@ -51,7 +51,13 @@ OPTIONS\n \tHash the contents as is, ignoring any input filter that would\n \thave been chosen by the attributes mechanism, including the end-of-line\n \tconversion. If the file is read from standard input then this\n-\tis always implied, unless the --path option is given.\n+\tis always implied, unless the `--path` option is given.\n+\n+--literally::\n+\tAllow `--stdin` to hash any garbage into a loose object which might not\n+\totherwise pass standard object parsing or git-fsck checks. Useful for\n+\tstress-testing Git itself or reproducing characteristics of corrupt or\n+\tbogus objects encountered in the wild.\n \n GIT\n ---\n-- \n2.4.0.319.g7a04823\n"},{"id":"260507","messageId":"1430724315-524-3-git-send-email-sunshine@sunshineco.com","threadId":"39231","inReplyTo":"1430724315-524-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 2/3] t1007: add hash-object --literally tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-04T07:25:14Z","receivedAt":"2015-05-04T07:25:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"git-hash-object learned a --literally option in 5ba9a93 (hash-object:\nadd --literally option, 2014-09-11). Check that --literally allows\nobject creation with a bogus type.\n\nAlso add a failing test demonstrating a crash (buffer overflow leading\nto stack corruption) when the bogus type is lengthy.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nAlthough the crash seems to have manifested[1] only on Mac OS X when\ntesting Karthik's \"cat-file --allow-unknown-type\" series, I made the\nbogus object type extremely long in the failing test included here in\norder to ensure that it manifests (hopefully) everywhere. At this\nlength, the crash manifest on Linux as well.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/268262/focus=268304\n\n t/t1007-hash-object.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex f83df8e..0e65577 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -201,4 +201,15 @@ test_expect_success 'corrupt tag' '\n \ttest_must_fail git hash-object -t tag --stdin </dev/null\n '\n \n+test_expect_success '--literally' '\n+\tt=1234567890 &&\n+\techo example | git hash-object -t $t --literally --stdin\n+'\n+\n+test_expect_failure '--literally with extra-long type' '\n+\tt=12345678901234567890123456789012345678901234567890 &&\n+\tt=\"$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\" &&\n+\techo example | git hash-object -t $t --literally --stdin\n+'\n+\n test_done\n-- \n2.4.0.319.g7a04823\n"},{"id":"260506","messageId":"1430724315-524-4-git-send-email-sunshine@sunshineco.com","threadId":"39231","inReplyTo":"1430724315-524-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-04T07:25:15Z","receivedAt":"2015-05-04T07:25:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"git-hash-object learned --literally in 5ba9a93 (hash-object: add\n--literally option, 2014-09-11) which can be used to craft a\ncorrupt/broken object of unknown type. When the user-provided type is\nparticularly long, it can overflow the relatively small stack-based\ncharacter array handed to write_sha1_file_prepare() by hash_sha1_file()\nand write_sha1_file(), leading to stack corruption (and crash).\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nThe composed 'hdr' is supposed to be NUL-terminated, and code which\naccesses 'hdr' expects the NUL to be included as part of its length.\nAlthough strbuf ensures that a NUL byte follows the string content, I\ntook the precaution of explicitly adding NUL when formulating 'hdr'\n\n    strbuf_addf(hdr, \"%s %lu%c\", type, len, '\\0');\n\nso that callers can just say hdr.len when the length is needed, rather\nthan having to remember to say hdr.len+1.\n\n\nI haven't fully convinced myself that this fix is appropriate since it\npenalizes _all_ callers of hash_sha1_file() and write_sha1_file() with\nan extra heap allocation (via strbuf), even though \"hash-object\n--literally\" is the only mechanism by which an overly-long object type\ncan arrive.\n\n sha1_file.c            | 21 ++++++++++-----------\n t/t1007-hash-object.sh |  2 +-\n 2 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88f06ba..6d3fa26 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2795,16 +2795,16 @@ void *read_object_with_reference(const unsigned char *sha1,\n \n static void write_sha1_file_prepare(const void *buf, unsigned long len,\n                                     const char *type, unsigned char *sha1,\n-                                    char *hdr, int *hdrlen)\n+\t\t\t\t    struct strbuf *hdr)\n {\n \tgit_SHA_CTX c;\n \n \t/* Generate the header */\n-\t*hdrlen = sprintf(hdr, \"%s %lu\", type, len)+1;\n+\tstrbuf_addf(hdr, \"%s %lu%c\", type, len, '\\0');\n \n \t/* Sha1.. */\n \tgit_SHA1_Init(&c);\n-\tgit_SHA1_Update(&c, hdr, *hdrlen);\n+\tgit_SHA1_Update(&c, hdr->buf, hdr->len);\n \tgit_SHA1_Update(&c, buf, len);\n \tgit_SHA1_Final(sha1, &c);\n }\n@@ -2865,9 +2865,8 @@ static int write_buffer(int fd, const void *buf, size_t len)\n int hash_sha1_file(const void *buf, unsigned long len, const char *type,\n                    unsigned char *sha1)\n {\n-\tchar hdr[32];\n-\tint hdrlen;\n-\twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n+\tstruct strbuf hdr = STRBUF_INIT;\n+\twrite_sha1_file_prepare(buf, len, type, sha1, &hdr);\n \treturn 0;\n }\n \n@@ -3005,18 +3004,18 @@ static int freshen_packed_object(const unsigned char *sha1)\n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n {\n \tunsigned char sha1[20];\n-\tchar hdr[32];\n-\tint hdrlen;\n+\tstruct strbuf hdr = STRBUF_INIT;\n \n-\t/* Normally if we have it in the pack then we do not bother writing\n+\t/*\n+\t * Normally if we have it in the pack then we do not bother writing\n \t * it out into .git/objects/??/?{38} file.\n \t */\n-\twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n+\twrite_sha1_file_prepare(buf, len, type, sha1, &hdr);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n \tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n \t\treturn 0;\n-\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n+\treturn write_loose_object(sha1, hdr.buf, hdr.len, buf, len, 0);\n }\n \n int force_object_loose(const unsigned char *sha1, time_t mtime)\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex 0e65577..7c3dcfb 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -206,7 +206,7 @@ test_expect_success '--literally' '\n \techo example | git hash-object -t $t --literally --stdin\n '\n \n-test_expect_failure '--literally with extra-long type' '\n+test_expect_success '--literally with extra-long type' '\n \tt=12345678901234567890123456789012345678901234567890 &&\n \tt=\"$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\" &&\n \techo example | git hash-object -t $t --literally --stdin\n-- \n2.4.0.319.g7a04823\n"},{"id":"260535","messageId":"xmqq4mnsteb8.fsf@gitster.dls.corp.google.com","threadId":"39231","inReplyTo":"1430724315-524-4-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T17:58:35Z","receivedAt":"2015-05-04T17:58:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> git-hash-object learned --literally in 5ba9a93 (hash-object: add\n> --literally option, 2014-09-11) which can be used to craft a\n> corrupt/broken object of unknown type. When the user-provided type is\n> particularly long, it can overflow the relatively small stack-based\n> character array handed to write_sha1_file_prepare() by hash_sha1_file()\n> and write_sha1_file(), leading to stack corruption (and crash).\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n\nThanks.\n\n> ---\n>\n> The composed 'hdr' is supposed to be NUL-terminated, and code which\n> accesses 'hdr' expects the NUL to be included as part of its length.\n> Although strbuf ensures that a NUL byte follows the string content, I\n> took the precaution of explicitly adding NUL when formulating 'hdr'\n>\n>     strbuf_addf(hdr, \"%s %lu%c\", type, len, '\\0');\n>\n> so that callers can just say hdr.len when the length is needed, rather\n> than having to remember to say hdr.len+1.\n\nThat is unnecessary and may turn out to be more confusing than it is\nworth in the long run; though I do not think it matters too much.\n\n> I haven't fully convinced myself that this fix is appropriate since it\n> penalizes _all_ callers of hash_sha1_file() and write_sha1_file() with\n> an extra heap allocation (via strbuf), even though \"hash-object\n> --literally\" is the only mechanism by which an overly-long object type\n> can arrive.\n\nI am moderately unhappy about fixing it this way for the exact\nreason you stated aboe.\n\nwrite_sha1_file_prepare() can stay the same as before, and we can\nuse a variant of hash_sha1_file() only when doing the --literally\nthing to use a special allocation.  Perhaps like this?\n\nThe patch was done on top of yours; it reverts the change to\nwrite_sha1_file_prepare() and its callers, and instead adds a\nseparate helper.  We may want to restructure the latter half of\nwrite_sha1_file() that does the freshen-or-write into a helper\nfunction to share more logic, though.\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 207b90c..ef383e1 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne\n \n \tif (strbuf_read(&buf, fd, 4096) < 0)\n \t\tret = -1;\n-\telse if (flags & HASH_WRITE_OBJECT)\n-\t\tret = write_sha1_file(buf.buf, buf.len, type, sha1);\n \telse\n-\t\tret = hash_sha1_file(buf.buf, buf.len, type, sha1);\n+\t\tret = hash_sha1_file_literally(&buf, type, sha1, flags);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\ndiff --git a/cache.h b/cache.h\nindex 3d3244b..a37423e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -874,6 +874,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned\n extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n+extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_noatime(const char *name);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6d3fa26..c8ab069 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2795,16 +2795,16 @@ void *read_object_with_reference(const unsigned char *sha1,\n \n static void write_sha1_file_prepare(const void *buf, unsigned long len,\n                                     const char *type, unsigned char *sha1,\n-\t\t\t\t    struct strbuf *hdr)\n+\t\t\t\t    char *hdr, int *hdrlen)\n {\n \tgit_SHA_CTX c;\n \n \t/* Generate the header */\n-\tstrbuf_addf(hdr, \"%s %lu%c\", type, len, '\\0');\n+\t*hdrlen = sprintf(hdr, \"%s %lu\", type, len)+1;\n \n \t/* Sha1.. */\n \tgit_SHA1_Init(&c);\n-\tgit_SHA1_Update(&c, hdr->buf, hdr->len);\n+\tgit_SHA1_Update(&c, hdr, *hdrlen);\n \tgit_SHA1_Update(&c, buf, len);\n \tgit_SHA1_Final(sha1, &c);\n }\n@@ -2865,8 +2865,9 @@ static int write_buffer(int fd, const void *buf, size_t len)\n int hash_sha1_file(const void *buf, unsigned long len, const char *type,\n                    unsigned char *sha1)\n {\n-\tstruct strbuf hdr = STRBUF_INIT;\n-\twrite_sha1_file_prepare(buf, len, type, sha1, &hdr);\n+\tchar hdr[32];\n+\tint hdrlen;\n+\twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n \treturn 0;\n }\n \n@@ -3004,18 +3005,43 @@ static int freshen_packed_object(const unsigned char *sha1)\n int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n {\n \tunsigned char sha1[20];\n-\tstruct strbuf hdr = STRBUF_INIT;\n+\tchar hdr[32];\n+\tint hdrlen;\n \n-\t/*\n-\t * Normally if we have it in the pack then we do not bother writing\n+\t/* Normally if we have it in the pack then we do not bother writing\n \t * it out into .git/objects/??/?{38} file.\n \t */\n-\twrite_sha1_file_prepare(buf, len, type, sha1, &hdr);\n+\twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n \tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n \t\treturn 0;\n-\treturn write_loose_object(sha1, hdr.buf, hdr.len, buf, len, 0);\n+\treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n+}\n+\n+int hash_sha1_file_literally(struct strbuf *buf, const char *type,\n+\t\t\t     unsigned char *sha1, unsigned flags)\n+{\n+\tstruct strbuf header = STRBUF_INIT;\n+\tint hdrlen, status = 0;\n+\n+\t/* type string, SP, %lu of the length plus NUL must fit this */\n+\tstrbuf_grow(&header, strlen(type) + 20);\n+\n+\twrite_sha1_file_prepare(buf->buf, buf->len, type, sha1,\n+\t\t\t\theader.buf, &hdrlen);\n+\n+\tif (!(flags & HASH_WRITE_OBJECT))\n+\t\tgoto cleanup;\n+\n+\tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n+\t\tgoto cleanup;\n+\tstatus = write_loose_object(sha1, header.buf, hdrlen,\n+\t\t\t\t    buf->buf, buf->len, 0);\n+\n+cleanup:\n+\tstrbuf_release(&header);\n+\treturn status;\n }\n \n int force_object_loose(const unsigned char *sha1, time_t mtime)\n"},{"id":"260536","messageId":"xmqqzj5krzpe.fsf@gitster.dls.corp.google.com","threadId":"39231","inReplyTo":"xmqq4mnsteb8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/3] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T17:59:25Z","receivedAt":"2015-05-04T17:59:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By the way, you would notice that \"if we have returnsha1, then copy\nthat in\" bit is not in the new literally codepath but still is in\nwrite_sha1_file().\n\nI do not think any caller passes a NULL as return_sha1 in today's\ncode, which made me curious.\n\nIt turns out to be a remnant of d6d3f9d0 (This implements the new\n\"recursive tree\" write-tree., 2005-04-09); before that change,\nwrite_sha1_file() did not have an ability to tell the caller what\nobject it wrote, and Linus made it optional for the callers when he\nadded the return_sha1[] out parameter, but all of its callers did\nwant the resulting object name.\n\nSo I think it is safe and sensible to do the following change\nregardless of \"hash-object --literally\" fix.\n\n sha1_file.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c8ab069..96e813f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3002,9 +3002,8 @@ static int freshen_packed_object(const unsigned char *sha1)\n \treturn find_pack_entry(sha1, &e) && freshen_file(e.p->pack_name);\n }\n \n-int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1)\n {\n-\tunsigned char sha1[20];\n \tchar hdr[32];\n \tint hdrlen;\n \n@@ -3012,8 +3011,6 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \t * it out into .git/objects/??/?{38} file.\n \t */\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n-\tif (returnsha1)\n-\t\thashcpy(returnsha1, sha1);\n \tif (freshen_loose_object(sha1) || freshen_packed_object(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n"},{"id":"260557","messageId":"1430775451-31130-1-git-send-email-gitster@pobox.com","threadId":"39231","inReplyTo":"1430724315-524-1-git-send-email-sunshine@sunshineco.com","subject":"[PATCH 0/4] \"hash-object --literally\" fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T21:37:27Z","receivedAt":"2015-05-04T21:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is a rework of Eric's fix (Thanks!), to avoid using strbuf all\nthe time, even when we are not doing \"hash-object --literally\".\n\nEric Sunshine (3):\n  git-hash-object.txt: document --literally option\n  write_sha1_file_prepare: fix buffer overrun with extra-long object type\n  t1007: add hash-object --literally tests\n\nJunio C Hamano (1):\n  write_sha1_file(): do not use a separate sha1[] array\n\n Documentation/git-hash-object.txt | 10 ++++++++--\n builtin/hash-object.c             |  4 +---\n cache.h                           |  1 +\n sha1_file.c                       | 32 +++++++++++++++++++++++++++-----\n t/t1007-hash-object.sh            | 11 +++++++++++\n 5 files changed, 48 insertions(+), 10 deletions(-)\n\n-- \n2.4.0-302-g6743426\n"},{"id":"260556","messageId":"1430775451-31130-2-git-send-email-gitster@pobox.com","threadId":"39231","inReplyTo":"1430775451-31130-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/4] git-hash-object.txt: document --literally option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T21:37:28Z","receivedAt":"2015-05-04T21:37:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\nDocument the git-hash-object --literally option added by 5ba9a93\n(hash-object: add --literally option, 2014-09-11).\n\nWhile here, also correct a minor typesetting oversight.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * As posted by Eric\n\n Documentation/git-hash-object.txt | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt\nindex 02c1f12..0c75f3b 100644\n--- a/Documentation/git-hash-object.txt\n+++ b/Documentation/git-hash-object.txt\n@@ -9,7 +9,7 @@ git-hash-object - Compute object ID and optionally creates a blob from a file\n SYNOPSIS\n --------\n [verse]\n-'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin] [--] <file>...\n+'git hash-object' [-t <type>] [-w] [--path=<file>|--no-filters] [--stdin [--literally]] [--] <file>...\n 'git hash-object' [-t <type>] [-w] --stdin-paths [--no-filters] < <list-of-paths>\n \n DESCRIPTION\n@@ -51,7 +51,13 @@ OPTIONS\n \tHash the contents as is, ignoring any input filter that would\n \thave been chosen by the attributes mechanism, including the end-of-line\n \tconversion. If the file is read from standard input then this\n-\tis always implied, unless the --path option is given.\n+\tis always implied, unless the `--path` option is given.\n+\n+--literally::\n+\tAllow `--stdin` to hash any garbage into a loose object which might not\n+\totherwise pass standard object parsing or git-fsck checks. Useful for\n+\tstress-testing Git itself or reproducing characteristics of corrupt or\n+\tbogus objects encountered in the wild.\n \n GIT\n ---\n-- \n2.4.0-302-g6743426\n"},{"id":"260558","messageId":"1430775451-31130-3-git-send-email-gitster@pobox.com","threadId":"39231","inReplyTo":"1430775451-31130-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T21:37:29Z","receivedAt":"2015-05-04T21:37:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\ngit-hash-object learned --literally in 5ba9a93 (hash-object: add\n--literally option, 2014-09-11) which can be used to craft a\ncorrupt/broken object of unknown type.\n\nWhen the user-provided type is particularly long, however, it can\noverflow the relatively small stack-based character array handed to\nwrite_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),\nleading to stack corruption (and crash).\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Tweaked and backported directly on top of 5ba9a93b (hash-object:\n   add --literally option, 2014-09-11) which is v2.2.0-rc0~88^2\n\n builtin/hash-object.c |  4 +---\n cache.h               |  1 +\n sha1_file.c           | 27 ++++++++++++++++++++++++++-\n 3 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 6158363..887a8ea 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne\n \n \tif (strbuf_read(&buf, fd, 4096) < 0)\n \t\tret = -1;\n-\telse if (flags & HASH_WRITE_OBJECT)\n-\t\tret = write_sha1_file(buf.buf, buf.len, type, sha1);\n \telse\n-\t\tret = hash_sha1_file(buf.buf, buf.len, type, sha1);\n+\t\tret = hash_sha1_file_literally(&buf, type, sha1, flags);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\ndiff --git a/cache.h b/cache.h\nindex dfa1a56..2da7740 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned\n extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n+extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_noatime(const char *name);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c08c0cb..0fe3f29 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2962,6 +2962,31 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n \n+int hash_sha1_file_literally(struct strbuf *buf, const char *type,\n+\t\t\t     unsigned char *sha1, unsigned flags)\n+{\n+\tstruct strbuf header = STRBUF_INIT;\n+\tint hdrlen, status = 0;\n+\n+\t/* type string, SP, %lu of the length plus NUL must fit this */\n+\tstrbuf_grow(&header, strlen(type) + 20);\n+\n+\twrite_sha1_file_prepare(buf->buf, buf->len, type, sha1,\n+\t\t\t\theader.buf, &hdrlen);\n+\n+\tif (!(flags & HASH_WRITE_OBJECT))\n+\t\tgoto cleanup;\n+\n+\tif (has_sha1_file(sha1))\n+\t\tgoto cleanup;\n+\tstatus = write_loose_object(sha1, header.buf, hdrlen,\n+\t\t\t\t    buf->buf, buf->len, 0);\n+\n+cleanup:\n+\tstrbuf_release(&header);\n+\treturn status;\n+}\n+\n int force_object_loose(const unsigned char *sha1, time_t mtime)\n {\n \tvoid *buf;\n-- \n2.4.0-302-g6743426\n"},{"id":"260560","messageId":"1430775451-31130-4-git-send-email-gitster@pobox.com","threadId":"39231","inReplyTo":"1430775451-31130-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/4] t1007: add hash-object --literally tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T21:37:30Z","receivedAt":"2015-05-04T21:37:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Eric Sunshine <sunshine@sunshineco.com>\n\ngit-hash-object learned a --literally option in 5ba9a93\n(hash-object: add --literally option, 2014-09-11). Check that\n--literally allows object creation with a bogus type, with two\ntype strings whose length is reasonably short and very long.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Almost as posted by Eric, but the fix happens earlier in the\n   series so both tests expect to succeed.\n\n t/t1007-hash-object.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex f83df8e..7c3dcfb 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -201,4 +201,15 @@ test_expect_success 'corrupt tag' '\n \ttest_must_fail git hash-object -t tag --stdin </dev/null\n '\n \n+test_expect_success '--literally' '\n+\tt=1234567890 &&\n+\techo example | git hash-object -t $t --literally --stdin\n+'\n+\n+test_expect_success '--literally with extra-long type' '\n+\tt=12345678901234567890123456789012345678901234567890 &&\n+\tt=\"$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\" &&\n+\techo example | git hash-object -t $t --literally --stdin\n+'\n+\n test_done\n-- \n2.4.0-302-g6743426\n"},{"id":"260559","messageId":"1430775451-31130-5-git-send-email-gitster@pobox.com","threadId":"39231","inReplyTo":"1430775451-31130-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/4] write_sha1_file(): do not use a separate sha1[] array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-04T21:37:31Z","receivedAt":"2015-05-04T21:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In the beginning, write_sha1_file() did not have a way to tell the\ncaller the name of the object it wrote to the caller.  This was\nchanged in d6d3f9d0 (This implements the new \"recursive tree\"\nwrite-tree., 2005-04-09) by adding the \"returnsha1\" parameter to the\nfunction so that the callers who are interested in the value can\noptionally pass a pointer to receive it.\n\nIt turns out that all callers do want to know the name of the object\nit just has written.  Nobody passes a NULL to this parameter, hence\nit is not necessary to use a separate sha1[] array to receive the\nresult from  write_sha1_file_prepare(), and copy the result to the\nreturnsha1 supplied by the caller.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is something I noticed while in the vicinity.\n\n sha1_file.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 0fe3f29..cec0ef2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2945,9 +2945,8 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \treturn move_temp_to_file(tmp_file, filename);\n }\n \n-int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1)\n {\n-\tunsigned char sha1[20];\n \tchar hdr[32];\n \tint hdrlen;\n \n@@ -2955,8 +2954,6 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \t * it out into .git/objects/??/?{38} file.\n \t */\n \twrite_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);\n-\tif (returnsha1)\n-\t\thashcpy(returnsha1, sha1);\n \tif (has_sha1_file(sha1))\n \t\treturn 0;\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n-- \n2.4.0-302-g6743426\n"},{"id":"260572","messageId":"CAPig+cS3f2XggxqbvX6Z2Da24QKLOg915Bf-bTVa+4oVzfhA1A@mail.gmail.com","threadId":"39231","inReplyTo":"1430775451-31130-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-05T00:13:18Z","receivedAt":"2015-05-05T00:13:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 4, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> From: Eric Sunshine <sunshine@sunshineco.com>\n\nThanks for re-rerolling this series. Considering that the only bits\nleft from me are the diagnosis and the (mostly intact) commit message,\nperhaps the authorship should be changed, or at the very least a big\n\"Helped-by: Junio\" added? Anyhow, a few minor comments below...\n\n> write_sha1_file_prepare: fix buffer overrun with extra-long object type\n\nAlthough the overrun happened in write_sha1_file_prepare(), that\nfunction is no longer the focus of the patch. Would make sense to\nrephrase the subject more generally as:\n\n    sha1_file: fix buffer overrun with extra-long object type\n\nor something.\n\nMore below.\n\n> git-hash-object learned --literally in 5ba9a93 (hash-object: add\n> --literally option, 2014-09-11) which can be used to craft a\n> corrupt/broken object of unknown type.\n>\n> When the user-provided type is particularly long, however, it can\n> overflow the relatively small stack-based character array handed to\n> write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),\n> leading to stack corruption (and crash).\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/cache.h b/cache.h\n> index dfa1a56..2da7740 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned\n>  extern int sha1_object_info(const unsigned char *, unsigned long *);\n>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n>  extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n> +extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);\n\nA few questions:\n\nWhat's the value of making the first argument of\nhash_sha1_file_literally() a strbuf rather than the two-argument buf &\nlen accepted by hash_sha1_file() and write_sha1_file()? Is the\ninconsistency warranted?\n\nWould it make sense to name the third argument \"sha1\" instead of\n\"return_sha1\" to match the argument name of hash_sha1_file()?\n\nAnd, as an aside, should your new patch 4/4 rename \"return_sha1\" to\n\"sha1\" in the write_sha1_file() prototype also?\n\n>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n>  extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n>  extern int git_open_noatime(const char *name);\n> diff --git a/sha1_file.c b/sha1_file.c\n> index c08c0cb..0fe3f29 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2962,6 +2962,31 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n>         return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n>  }\n>\n> +int hash_sha1_file_literally(struct strbuf *buf, const char *type,\n> +                            unsigned char *sha1, unsigned flags)\n> +{\n> +       struct strbuf header = STRBUF_INIT;\n> +       int hdrlen, status = 0;\n> +\n> +       /* type string, SP, %lu of the length plus NUL must fit this */\n> +       strbuf_grow(&header, strlen(type) + 20);\n\nA couple comments:\n\nFirst, given that the largest 64-bit unsigned long value\n(18,446,744,073,709,551,615) is 20 characters, do we want to be really\npedantic and add 22 instead of 20?\n\nSecond, is strbuf overkill in this situation when a simple\nxmalloc()/free() would do?\n\n> +       write_sha1_file_prepare(buf->buf, buf->len, type, sha1,\n> +                               header.buf, &hdrlen);\n> +\n> +       if (!(flags & HASH_WRITE_OBJECT))\n> +               goto cleanup;\n> +\n> +       if (has_sha1_file(sha1))\n> +               goto cleanup;\n> +       status = write_loose_object(sha1, header.buf, hdrlen,\n> +                                   buf->buf, buf->len, 0);\n> +\n> +cleanup:\n> +       strbuf_release(&header);\n> +       return status;\n> +}\n> +\n>  int force_object_loose(const unsigned char *sha1, time_t mtime)\n>  {\n>         void *buf;\n> --\n> 2.4.0-302-g6743426\n"},{"id":"260574","messageId":"xmqq4mnrg94p.fsf@gitster.dls.corp.google.com","threadId":"39231","inReplyTo":"CAPig+cS3f2XggxqbvX6Z2Da24QKLOg915Bf-bTVa+4oVzfhA1A@mail.gmail.com","subject":"Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-05T00:28:54Z","receivedAt":"2015-05-05T00:28:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> +extern int hash_sha1_file_literally(struct strbuf *buf, const char\n>> *type, unsigned char *return_sha1, unsigned flags);\n>\n> A few questions:\n>\n> What's the value of making the first argument of\n> hash_sha1_file_literally() a strbuf rather than the two-argument\n> buf & len accepted by hash_sha1_file() and write_sha1_file()? Is\n> the inconsistency warranted?\n\nI do not care either way, as this is really meant to be a\nsingle-purpose wrapper for a single caller.\n\n> Would it make sense to name the third argument \"sha1\" instead of\n> \"return_sha1\" to match the argument name of hash_sha1_file()?\n>\n> And, as an aside, should your new patch 4/4 rename \"return_sha1\" to\n> \"sha1\" in the write_sha1_file() prototype also?\n\nBecause most of the read-cache.c functions take object name and do\nsomething about it, but these small number of functions compute and\nreturn object name, I actually think it is a good way to keep\n\"return\" in the name to remind those who are writing or reading\ncallers.\n\n>> +       /* type string, SP, %lu of the length plus NUL must fit this */\n>> +       strbuf_grow(&header, strlen(type) + 20);\n>\n> A couple comments:\n>\n> First, given that the largest 64-bit unsigned long value\n> (18,446,744,073,709,551,615) is 20 characters, do we want to be really\n> pedantic and add 22 instead of 20?\n\n32 is fine ;-)\n\n> Second, is strbuf overkill in this situation when a simple\n> xmalloc()/free() would do?\n\nI think underneath the number of xmalloc()/free() calls are the\nsame.  The code that uses strbuf abstraction is easier to read, I\nwould think.\n"},{"id":"260597","messageId":"xmqqvbg7dja6.fsf@gitster.dls.corp.google.com","threadId":"39231","inReplyTo":"CAPig+cS3f2XggxqbvX6Z2Da24QKLOg915Bf-bTVa+4oVzfhA1A@mail.gmail.com","subject":"Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-05T17:30:09Z","receivedAt":"2015-05-05T17:30:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Mon, May 4, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> From: Eric Sunshine <sunshine@sunshineco.com>\n>\n> Thanks for re-rerolling this series. Considering that the only bits\n> left from me are the diagnosis and the (mostly intact) commit message,\n> perhaps the authorship should be changed, or at the very least a big\n> \"Helped-by: Junio\" added? Anyhow, a few minor comments below...\n\nI am a bit too lazy to take the ownership, so I decided only to take\nthe blame ;-)\n\nHere is a replacement; all the other patches stay the same.\n\n-- >8 --\nFrom: Eric Sunshine <sunshine@sunshineco.com>\nDate: Mon, 4 May 2015 03:25:15 -0400\nSubject: [PATCH] hash-object --literally: fix buffer overrun with extra-long object type\n\n\"hash-object\" learned in 5ba9a93 (hash-object: add --literally\noption, 2014-09-11) to allow crafting a corrupt/broken object of\nunknown type.\n\nWhen the user-provided type is particularly long, however, it can\noverflow the relatively small stack-based character array handed to\nwrite_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),\nleading to stack corruption (and crash).  Introduce a custom helper\nto allow arbitrarily long typenames just for \"hash-object --literally\".\n\n[jc: Eric's original used a strbuf in the more common codepaths, and\nI rewrote it to avoid penalizing the non-literally code. Bugs are mine]\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/hash-object.c |  4 +---\n cache.h               |  1 +\n sha1_file.c           | 21 +++++++++++++++++++++\n 3 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 6158363..17e8bfdc 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne\n \n \tif (strbuf_read(&buf, fd, 4096) < 0)\n \t\tret = -1;\n-\telse if (flags & HASH_WRITE_OBJECT)\n-\t\tret = write_sha1_file(buf.buf, buf.len, type, sha1);\n \telse\n-\t\tret = hash_sha1_file(buf.buf, buf.len, type, sha1);\n+\t\tret = hash_sha1_file_literally(buf.buf, buf.len, type, sha1, flags);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\ndiff --git a/cache.h b/cache.h\nindex dfa1a56..e037cad 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned\n extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n+extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n extern int git_open_noatime(const char *name);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c08c0cb..dc940e6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2962,6 +2962,27 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n \treturn write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n }\n \n+int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type,\n+\t\t\t     unsigned char *sha1, unsigned flags)\n+{\n+\tchar *header;\n+\tint hdrlen, status = 0;\n+\n+\t/* type string, SP, %lu of the length plus NUL must fit this */\n+\theader = xmalloc(strlen(type) + 32);\n+\twrite_sha1_file_prepare(buf, len, type, sha1, header, &hdrlen);\n+\n+\tif (!(flags & HASH_WRITE_OBJECT))\n+\t\tgoto cleanup;\n+\tif (has_sha1_file(sha1))\n+\t\tgoto cleanup;\n+\tstatus = write_loose_object(sha1, header, hdrlen, buf, len, 0);\n+\n+cleanup:\n+\tfree(header);\n+\treturn status;\n+}\n+\n int force_object_loose(const unsigned char *sha1, time_t mtime)\n {\n \tvoid *buf;\n-- \n2.4.0-311-gf1d9b8d\n"},{"id":"260610","messageId":"CAPig+cSZ0ggYJkKoeHZeh5vSVfuzZ54pxx8jYXNOm_wCr8KKPA@mail.gmail.com","threadId":"39231","inReplyTo":"xmqqvbg7dja6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-05T18:49:50Z","receivedAt":"2015-05-05T18:49:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 5, 2015 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> From: Eric Sunshine <sunshine@sunshineco.com>\n> Date: Mon, 4 May 2015 03:25:15 -0400\n> Subject: [PATCH] hash-object --literally: fix buffer overrun with extra-long object type\n>\n> \"hash-object\" learned in 5ba9a93 (hash-object: add --literally\n> option, 2014-09-11) to allow crafting a corrupt/broken object of\n> unknown type.\n>\n> When the user-provided type is particularly long, however, it can\n> overflow the relatively small stack-based character array handed to\n> write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),\n> leading to stack corruption (and crash).  Introduce a custom helper\n> to allow arbitrarily long typenames just for \"hash-object --literally\".\n>\n> [jc: Eric's original used a strbuf in the more common codepaths, and\n> I rewrote it to avoid penalizing the non-literally code. Bugs are mine]\n\nThanks for re-rolling again. The amended Subject: works nicely now,\nand the addition to the commit message makes sense. Also, the code\nchanges in response to the minor questions I raised[1] all look good.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/268306/focus=268374\n\n(rest of patch left unsnipped)\n\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/builtin/hash-object.c b/builtin/hash-object.c\n> index 6158363..17e8bfdc 100644\n> --- a/builtin/hash-object.c\n> +++ b/builtin/hash-object.c\n> @@ -22,10 +22,8 @@ static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigne\n>\n>         if (strbuf_read(&buf, fd, 4096) < 0)\n>                 ret = -1;\n> -       else if (flags & HASH_WRITE_OBJECT)\n> -               ret = write_sha1_file(buf.buf, buf.len, type, sha1);\n>         else\n> -               ret = hash_sha1_file(buf.buf, buf.len, type, sha1);\n> +               ret = hash_sha1_file_literally(buf.buf, buf.len, type, sha1, flags);\n>         strbuf_release(&buf);\n>         return ret;\n>  }\n> diff --git a/cache.h b/cache.h\n> index dfa1a56..e037cad 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned\n>  extern int sha1_object_info(const unsigned char *, unsigned long *);\n>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n>  extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n> +extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n>  extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n>  extern int git_open_noatime(const char *name);\n> diff --git a/sha1_file.c b/sha1_file.c\n> index c08c0cb..dc940e6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2962,6 +2962,27 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign\n>         return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);\n>  }\n>\n> +int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type,\n> +                            unsigned char *sha1, unsigned flags)\n> +{\n> +       char *header;\n> +       int hdrlen, status = 0;\n> +\n> +       /* type string, SP, %lu of the length plus NUL must fit this */\n> +       header = xmalloc(strlen(type) + 32);\n> +       write_sha1_file_prepare(buf, len, type, sha1, header, &hdrlen);\n> +\n> +       if (!(flags & HASH_WRITE_OBJECT))\n> +               goto cleanup;\n> +       if (has_sha1_file(sha1))\n> +               goto cleanup;\n> +       status = write_loose_object(sha1, header, hdrlen, buf, len, 0);\n> +\n> +cleanup:\n> +       free(header);\n> +       return status;\n> +}\n> +\n>  int force_object_loose(const unsigned char *sha1, time_t mtime)\n>  {\n>         void *buf;\n> --\n> 2.4.0-311-gf1d9b8d\n"}]}