{"thread":{"id":"38138","subject":"[PATCH] fsck: properly bound \"invalid tag name\" error message","startedAt":"2014-12-08T05:48:13Z","lastAt":"2014-12-08T14:17:55Z","messageCount":10,"participants":["Jeff King","Johannes Schindelin","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253290","messageId":"20141208054812.GA30154@peff.net","threadId":"38138","inReplyTo":null,"subject":"[PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-08T05:48:13Z","receivedAt":"2014-12-08T05:48:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we detect an invalid tag-name header in a tag object,\nlike, \"tag foo bar\\n\", we feed the pointer starting at \"foo\nbar\" to a printf \"%s\" formatter. This shows the name, as we\nwant, but then it keeps printing the rest of the tag buffer,\nrather than stopping at the end of the line.\n\nOur tests did not notice because they look only for the\nmatching line, but the bug is that we print much more than\nwe wanted to. So we also adjust the test to be more exact.\n\nNote that when fscking tags with \"index-pack --strict\", this\nis even worse. index-pack does not add a trailing\nNUL-terminator after the object, so we may actually read\npast the buffer and print uninitialized memory. Running\nt5302 with valgrind does notice the bug for that reason.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm generally nervous about adding too-specific stderr wording or\nformatting to our tests, as I do not want them to be brittle. But I do\nnot actually think this is substantially different than what other fsck\ntests do (i.e., they are already grepping for _half_ the wording\nalready, so if it changes, they are likely to break, too).\n\nIf we care, the test can check test_line_count or similar to make sure\nthere isn't extra data. But I think the way I have written it below is a\nlot easier for a reader coming later to understand what is going on.\n\n fsck.c          | 3 ++-\n t/t1450-fsck.sh | 8 ++++++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 2fffa43..88c92e8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -423,7 +423,8 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,\n \t}\n \tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n \tif (check_refname_format(sb.buf, 0))\n-\t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %s\", buffer);\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %.*s\",\n+\t\t\t   (int)(eol - buffer), buffer);\n \tbuffer = eol + 1;\n \n \tif (!skip_prefix(buffer, \"tagger \", &buffer))\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 019fddd..57ccce5 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -229,8 +229,12 @@ test_expect_success 'tag with incorrect tag name & missing tagger' '\n \techo $tag >.git/refs/tags/wrong &&\n \ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n \tgit fsck --tags 2>out &&\n-\tgrep \"invalid .tag. name\" out &&\n-\tgrep \"expected .tagger. line\" out\n+\n+\tcat >expect <<-EOF &&\n+\twarning in tag $tag: invalid '\\''tag'\\'' name: wrong name format\n+\twarning in tag $tag: invalid format - expected '\\''tagger'\\'' line\n+\tEOF\n+\ttest_cmp expect out\n '\n \n test_expect_success 'tag with bad tagger' '\n-- \n2.2.0.390.gf60752d\n"},{"id":"253291","messageId":"20141208055706.GA30207@peff.net","threadId":"38138","inReplyTo":"20141208054812.GA30154@peff.net","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-08T05:57:06Z","receivedAt":"2014-12-08T05:57:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 08, 2014 at 12:48:12AM -0500, Jeff King wrote:\n\n> Note that when fscking tags with \"index-pack --strict\", this\n> is even worse. index-pack does not add a trailing\n> NUL-terminator after the object, so we may actually read\n> past the buffer and print uninitialized memory. Running\n> t5302 with valgrind does notice the bug for that reason.\n\nThis merits an additional note (but fortunately not a patch :) ).\n\nAfter writing the above, I thought for a moment that we might actually\nread past the end of the buffer in some cases, but I convinced myself\notherwise. And I think Dscho and I might have even had this conversation\noff-list a while ago, but I think it is worth pointing out so that\nnobody else has to dig into it.\n\nFor the most part, we are fine because we parse the object\nleft-to-right, and barf as soon as we see something unusual (and for\nthis reason, fsck_commit_buffer is also fine). The two suspicious places\nare:\n\n  1. We call strchr(buffer, '\\n'), which looks like it could read\n     unbounded when \"buffer\" is not NUL-terminated. However, early in\n     the function we confirm that it contains \"\\n\\n\", and we will not\n     have parsed past that here. Therefore we know that we will always\n     hit a newline.\n\n  2. After finding and parsing a line whose trailing newline is marked\n     by \"eol\", we then set \"buffer = eol + 1\". This would be wrong if\n     eol is at the very end of the buffer (the next step would then\n     start reading uninitialized memory).\n\n     But again we are saved by the \"\\n\\n\" check. The strchr will always\n     find the first, so we know that we have at least one character\n     after it (and that character is a newline, which cannot be the\n     start of a new header, which will cause us to stop parsing).\n\nI do admit that I am tempted to teach index-pack to always NUL-terminate\nobjects in memory that we feed to fsck, just to be on the safe side. It\ndoesn't cost much, and could prevent a silly mistake (either in the\nfuture, or one that I missed in my analysis). The fsck code otherwise\ngenerally expects to get the output of read_sha1_file, which has the\nsafety-NUL appended.\n\n-Peff\n"},{"id":"253299","messageId":"alpine.DEB.1.00.1412081200460.13845@s15462909.onlinehome-server.info","threadId":"38138","inReplyTo":"20141208054812.GA30154@peff.net","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-08T11:01:13Z","receivedAt":"2014-12-08T11:01:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 8 Dec 2014, Jeff King wrote:\n\n> When we detect an invalid tag-name header in a tag object,\n> like, \"tag foo bar\\n\", we feed the pointer starting at \"foo\n> bar\" to a printf \"%s\" formatter. This shows the name, as we\n> want, but then it keeps printing the rest of the tag buffer,\n> rather than stopping at the end of the line.\n> \n> Our tests did not notice because they look only for the\n> matching line, but the bug is that we print much more than\n> we wanted to. So we also adjust the test to be more exact.\n\nGood catch, thank you!\n\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nCiao,\nDscho\n"},{"id":"253300","messageId":"alpine.DEB.1.00.1412081201180.13845@s15462909.onlinehome-server.info","threadId":"38138","inReplyTo":"20141208055706.GA30207@peff.net","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-08T11:17:10Z","receivedAt":"2014-12-08T11:17:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 8 Dec 2014, Jeff King wrote:\n\n> On Mon, Dec 08, 2014 at 12:48:12AM -0500, Jeff King wrote:\n> \n> > Note that when fscking tags with \"index-pack --strict\", this is even\n> > worse. index-pack does not add a trailing NUL-terminator after the\n> > object, so we may actually read past the buffer and print\n> > uninitialized memory. Running t5302 with valgrind does notice the bug\n> > for that reason.\n> \n> This merits an additional note (but fortunately not a patch :) ).\n\nWehell... your note about index-pack is definitely worth an additional\nnote, and - as you suggest later - probably also a patch. When I\nstarted working on the fsck code handling tags, I did note that the tag\nbuffers were not NUL terminated, mainly due to crashes that were really\ndifficult to debug (I had to resort to the sleep loop trick to attach to a\nprocess spawned by git-push) after I added code that assumed the buffers\nto be NUL terminated.\n\n> After writing the above, I thought for a moment that we might actually\n> read past the end of the buffer in some cases, but I convinced myself\n> otherwise. And I think Dscho and I might have even had this conversation\n> off-list a while ago, but I think it is worth pointing out so that\n> nobody else has to dig into it.\n\nYep, we discussed this quite a bit. I argued that the safest thing is\n*not* to assume that the buffers are NUL terminated because it was not\nobvious to me how to guarantee NUL-terminated object buffers (because the\ncommit objects are reused by the fsck machinery, not re-read).\n\n> For the most part, we are fine because we parse the object\n> left-to-right, and barf as soon as we see something unusual (and for\n> this reason, fsck_commit_buffer is also fine). The two suspicious places\n> are:\n> \n>   1. We call strchr(buffer, '\\n'), which looks like it could read\n>      unbounded when \"buffer\" is not NUL-terminated. However, early in\n>      the function we confirm that it contains \"\\n\\n\", and we will not\n>      have parsed past that here. Therefore we know that we will always\n>      hit a newline.\n\nFor reference, this is the code:\n\n\thttps://github.com/git/git/blob/c18b86734113ee2aeb0e140c922c8fbd4accc860/fsck.c#L241-L259\n\nbeing called by:\n\n\thttps://github.com/git/git/blob/c18b86734113ee2aeb0e140c922c8fbd4accc860/fsck.c#L308\n\nand\n\n\thttps://github.com/git/git/blob/c18b86734113ee2aeb0e140c922c8fbd4accc860/fsck.c#L387\n\n>   2. After finding and parsing a line whose trailing newline is marked\n>      by \"eol\", we then set \"buffer = eol + 1\". This would be wrong if\n>      eol is at the very end of the buffer (the next step would then\n>      start reading uninitialized memory).\n> \n>      But again we are saved by the \"\\n\\n\" check. The strchr will always\n>      find the first, so we know that we have at least one character\n>      after it (and that character is a newline, which cannot be the\n>      start of a new header, which will cause us to stop parsing).\n\nExactly. It is unfortunately a little too brittle for my taste, because it\nwould be relatively easy to break the assumption without noticing. For\nexample, in my upcoming patch series allowing to turn specific fsck errors\ninto mere warnings, it would have been potentially very dangerous to allow\ndemoting that error (no end of header found, NUL inside header) to a\nwarning - because that would have allowed the code to go beyond the\nbuffer.\n\nHowever...\n\n> I do admit that I am tempted to teach index-pack to always NUL-terminate\n> objects in memory that we feed to fsck, just to be on the safe side. It\n> doesn't cost much, and could prevent a silly mistake (either in the\n> future, or one that I missed in my analysis). The fsck code otherwise\n> generally expects to get the output of read_sha1_file, which has the\n> safety-NUL appended.\n\nIf we do that, we have to NUL-terminate all of the objects, correct? I\nmean, even the blobs and the trees and stuff, because we cannot know\nbeforehand what type of object we're gonna read, right?\n\nCiao,\nDscho\n"},{"id":"253302","messageId":"20141208112240.GA3191@peff.net","threadId":"38138","inReplyTo":"alpine.DEB.1.00.1412081201180.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-08T11:22:40Z","receivedAt":"2014-12-08T11:22:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 08, 2014 at 12:17:10PM +0100, Johannes Schindelin wrote:\n\n> > I do admit that I am tempted to teach index-pack to always NUL-terminate\n> > objects in memory that we feed to fsck, just to be on the safe side. It\n> > doesn't cost much, and could prevent a silly mistake (either in the\n> > future, or one that I missed in my analysis). The fsck code otherwise\n> > generally expects to get the output of read_sha1_file, which has the\n> > safety-NUL appended.\n> \n> If we do that, we have to NUL-terminate all of the objects, correct? I\n> mean, even the blobs and the trees and stuff, because we cannot know\n> beforehand what type of object we're gonna read, right?\n\nI think that is right. It should be a pretty simple change. It looks\nlike we already use xmallocz when creating deltas, and we just need to\nhandle regular objects. I think it could be as simple as this one-liner,\nbut I didn't test anything:\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a369f55..390845d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -447,7 +447,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \tif (type == OBJ_BLOB && size > big_file_threshold)\n \t\tbuf = fixed_buf;\n \telse\n-\t\tbuf = xmalloc(size);\n+\t\tbuf = xmallocz(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n\n-Peff\n"},{"id":"253301","messageId":"20141208112835.GA15919@lanh","threadId":"38138","inReplyTo":"20141208055706.GA30207@peff.net","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-12-08T11:28:35Z","receivedAt":"2014-12-08T11:28:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:\n> I do admit that I am tempted to teach index-pack to always NUL-terminate\n> objects in memory that we feed to fsck, just to be on the safe side. It\n> doesn't cost much, and could prevent a silly mistake (either in the\n> future, or one that I missed in my analysis).\n\nI think I'm missing a \"but..\" here. Maybe \"but I didn't have\ntime\". The change looks simple enough. The remaining *alloc in\nindex-pack is either for arrays, or already NUL-terminated\n(patch_delta), or does explicit boundary check (compare_objects).\n\nIt may be interesting to go over `git grep alloc\\(` and see if we\nshould use the allocz version instead. I think in some place we do\nxmalloc(len + 1) which could be replaced with xmallocz(len)\n\n-- 8< --\nSubject: [PATCH] index-pack: terminate object buffers with NUL\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/index-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a369f55..4632117 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -447,7 +447,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \tif (type == OBJ_BLOB && size > big_file_threshold)\n \t\tbuf = fixed_buf;\n \telse\n-\t\tbuf = xmalloc(size);\n+\t\tbuf = xmallocz(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n@@ -552,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,\n \tgit_zstream stream;\n \tint status;\n \n-\tdata = xmalloc(consume ? 64*1024 : obj->size);\n+\tdata = xmallocz(consume ? 64*1024 : obj->size);\n \tinbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n \n \tmemset(&stream, 0, sizeof(stream));\n-- \n2.2.0.60.gb7b3c64\n\n-- 8< --\n"},{"id":"253303","messageId":"alpine.DEB.1.00.1412081232500.13845@s15462909.onlinehome-server.info","threadId":"38138","inReplyTo":"20141208112835.GA15919@lanh","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-08T11:35:27Z","receivedAt":"2014-12-08T11:35:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Mon, 8 Dec 2014, Duy Nguyen wrote:\n\n> On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:\n> > I do admit that I am tempted to teach index-pack to always NUL-terminate\n> > objects in memory that we feed to fsck, just to be on the safe side. It\n> > doesn't cost much, and could prevent a silly mistake (either in the\n> > future, or one that I missed in my analysis).\n> \n> I think I'm missing a \"but..\" here.\n\nThe \"but...\"s I have are:\n\n1) we potentially waste space, and\n\n2) I would like to make really certain, preferably with static analysis,\n   that fsck_object() only receives buffers that are NUL terminated, and\n   that no call path is missed.\n\nThe patch looks good, of course, but I lack the broad overview of Git's\nsource code - it has been years since I was familiar enough with it to\nknow the places touching particular functions from the top of my head.\n\nCiao,\nDscho\n"},{"id":"253305","messageId":"20141208114740.GA16658@peff.net","threadId":"38138","inReplyTo":"alpine.DEB.1.00.1412081232500.13845@s15462909.onlinehome-server.info","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-08T11:47:40Z","receivedAt":"2014-12-08T11:47:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 08, 2014 at 12:35:27PM +0100, Johannes Schindelin wrote:\n\n> On Mon, 8 Dec 2014, Duy Nguyen wrote:\n> \n> > On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:\n> > > I do admit that I am tempted to teach index-pack to always NUL-terminate\n> > > objects in memory that we feed to fsck, just to be on the safe side. It\n> > > doesn't cost much, and could prevent a silly mistake (either in the\n> > > future, or one that I missed in my analysis).\n> > \n> > I think I'm missing a \"but..\" here.\n> \n> The \"but...\"s I have are:\n> \n> 1) we potentially waste space, and\n\nI think this can be ignored. It's 1 byte per object, and only while we\nkeep the object in RAM. Also, we already do it for buffers read from\nread_sha1_file, so when you run \"git log\" every commit buffer we keep in\nRAM is already doing this (and has been since basically day one).\n\n> 2) I would like to make really certain, preferably with static analysis,\n>    that fsck_object() only receives buffers that are NUL terminated, and\n>    that no call path is missed.\n\nI know this is not as good as a real static analysis, but I was\nconcerned about this exact thing about a year ago (I think in relation\nto commit parsing for pretty-printing) and traced all of the paths\nthrough which you can get an object; they all end up in the same few\ncode paths that all xmallocz: unpack_sha1_file for loose objects,\nunpack_compressed_entry for pack bases, and patch_delta for deltas.\n\nIndex-pack and unpack-objects are the odd men out here because they are\nprocessing objects that are not actually in the repository yet. I think\nthe spots Duy pointed out probably cover index-pack. It looks like\nbuiltin/unpack-objects.c:get_data needs the same treatment.\n\nI know that Duy mentioned a while ago killing off unpack-objects and\nrolling its functionality into index-pack. That would be a very nice\nthing to do if somebody can find the time. There's a fair bit of\nduplication, and index-pack receives a lot more attention (so it's\nfaster, and probably more robust against weird incoming packs).\n\n-Peff\n"},{"id":"253308","messageId":"alpine.DEB.1.00.1412081431540.13845@s15462909.onlinehome-server.info","threadId":"38138","inReplyTo":"20141208114740.GA16658@peff.net","subject":"Re: [PATCH] fsck: properly bound \"invalid tag name\" error message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-08T13:46:28Z","receivedAt":"2014-12-08T13:46:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 8 Dec 2014, Jeff King wrote:\n\n> On Mon, Dec 08, 2014 at 12:35:27PM +0100, Johannes Schindelin wrote:\n> \n> > On Mon, 8 Dec 2014, Duy Nguyen wrote:\n> > \n> > > On Mon, Dec 08, 2014 at 12:57:06AM -0500, Jeff King wrote:\n> > > > I do admit that I am tempted to teach index-pack to always NUL-terminate\n> > > > objects in memory that we feed to fsck, just to be on the safe side. It\n> > > > doesn't cost much, and could prevent a silly mistake (either in the\n> > > > future, or one that I missed in my analysis).\n> > > \n> > > I think I'm missing a \"but..\" here.\n> > \n> > The \"but...\"s I have are:\n> > \n> > 1) we potentially waste space, and\n> \n> I think this can be ignored. It's 1 byte per object, and only while we\n> keep the object in RAM. Also, we already do it for buffers read from\n> read_sha1_file, so when you run \"git log\" every commit buffer we keep in\n> RAM is already doing this (and has been since basically day one).\n\nFine with me.\n\n> > 2) I would like to make really certain, preferably with static analysis,\n> >    that fsck_object() only receives buffers that are NUL terminated, and\n> >    that no call path is missed.\n> \n> I know this is not as good as a real static analysis, but I was\n> concerned about this exact thing about a year ago (I think in relation\n> to commit parsing for pretty-printing) and traced all of the paths\n> through which you can get an object; they all end up in the same few\n> code paths that all xmallocz: unpack_sha1_file for loose objects,\n> unpack_compressed_entry for pack bases, and patch_delta for deltas.\n\nThank you for sharing the analysis. This is exactly what I was looking\nfor.\n\n> Index-pack and unpack-objects are the odd men out here because they are\n> processing objects that are not actually in the repository yet. I think\n> the spots Duy pointed out probably cover index-pack. It looks like\n> builtin/unpack-objects.c:get_data needs the same treatment.\n\nI just started working on that. To see the progress, please have a look\nhere:\n\n\thttps://github.com/dscho/git/pull/5\n\nCiao,\nDscho\n"},{"id":"253336","messageId":"alpine.DEB.1.00.1412081511390.13845@s15462909.onlinehome-server.info","threadId":"38138","inReplyTo":"20141208112835.GA15919@lanh","subject":"[PATCH v2] index-pack: terminate object buffers with NUL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-12-08T14:17:55Z","receivedAt":"2014-12-08T14:17:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Duy Nguyen <pclouds@gmail.com>\n\nWe have some tricky checks in fsck that rely on a side effect of\nrequire_end_of_header(), and would otherwise easily run outside\nnon-NUL-terminated buffers. This is a bit brittle, so let's make sure\nthat only NUL-terminated buffers are passed around to begin with.\n\nJeff \"Peff\" King contributed the detailed analysis which call paths are\ninvolved and pointed out that we also have to patch the get_data()\nfunction in unpack-objects.c, which is what Johannes \"Dscho\" Schindelin\nimplemented.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nAnalyzed-by: Jeff King <peff@peff.net>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tOn Mon, 8 Dec 2014, Duy Nguyen wrote:\n\n\t> Subject: [PATCH] index-pack: terminate object buffers with NUL\n\t> \n\t> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\n\tHere is a patch that is updated with Peff's suggested\n\tunpack-object.c:get_data change.\n\n\tWhile it is not as good as Peff's analysis, I can provide an\n\tadditional data point: the test suite passes cleanly even with\n\n\t\thttps://github.com/dscho/git/commit/567ad592\n\n\tapplied (and with 567ad592, but without below changes, at least\n\tt1050 does not pass).\n\n builtin/index-pack.c     | 4 ++--\n builtin/unpack-objects.c | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex a369f55..4632117 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -447,7 +447,7 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \tif (type == OBJ_BLOB && size > big_file_threshold)\n \t\tbuf = fixed_buf;\n \telse\n-\t\tbuf = xmalloc(size);\n+\t\tbuf = xmallocz(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init(&stream);\n@@ -552,7 +552,7 @@ static void *unpack_data(struct object_entry *obj,\n \tgit_zstream stream;\n \tint status;\n \n-\tdata = xmalloc(consume ? 64*1024 : obj->size);\n+\tdata = xmallocz(consume ? 64*1024 : obj->size);\n \tinbuf = xmalloc((len < 64*1024) ? len : 64*1024);\n \n \tmemset(&stream, 0, sizeof(stream));\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 855d94b..ac66672 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -91,7 +91,7 @@ static void use(int bytes)\n static void *get_data(unsigned long size)\n {\n \tgit_zstream stream;\n-\tvoid *buf = xmalloc(size);\n+\tvoid *buf = xmallocz(size);\n \n \tmemset(&stream, 0, sizeof(stream));\n \n-- \n1.8.4.msysgit.0.dirty\n"}]}