{"thread":{"id":"46781","subject":"[PATCH 0/2] fix read past end of array in alternates files","startedAt":"2017-09-18T15:51:07Z","lastAt":"2017-09-20T02:44:56Z","messageCount":14,"participants":["Jeff King","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"328299","messageId":"20170918155059.54f7z6cnrl47f5el@sigill.intra.peff.net","threadId":"46781","inReplyTo":null,"subject":"[PATCH 0/2] fix read past end of array in alternates files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-18T15:51:00Z","receivedAt":"2017-09-18T15:51:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes a regression in v2.11.1 where we might read past the\nend of an mmap'd buffer. It was introduced in cf3c635210, but I didn't\nbase the patch on there, for a few reasons:\n\n 1. There's a trivial conflict when merging up (because of\n git_open_noatime() becoming just git_open() in the inerim).\n\n 2. The reproduction advice relies on our SANITIZE Makefile knob, which\n didn't exist back then.\n\n 3. The second patch does not apply there because we don't have\n warn_on_fopen_errors(). Though admittedly it could be applied\n separately after merging up; it's just a clean-up on top.\n\nIt does apply on the current \"maint\".\n\n [1/2]: read_info_alternates: read contents into strbuf\n [2/2]: read_info_alternates: warn on non-trivial errors\n\n sha1_file.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\n-Peff\n"},{"id":"328300","messageId":"20170918155429.hgql4ngayun5nbyd@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170918155059.54f7z6cnrl47f5el@sigill.intra.peff.net","subject":"[PATCH 1/2] read_info_alternates: read contents into strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-18T15:54:29Z","receivedAt":"2017-09-18T15:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The link_alt_odb_entries() function has always taken a\nptr/len pair as input. Until cf3c635210 (alternates: accept\ndouble-quoted paths, 2016-12-12), we made a copy of those\nbytes in a string. But after that commit, we switched to\nparsing the input left-to-right, and we ignore \"len\"\ntotally, instead reading until we hit a NUL.\n\nThis has mostly gone unnoticed for a few reasons:\n\n 1. All but one caller passes a NUL-terminated string, with\n \"len\" pointing to the NUL.\n\n 2. The remaining caller, read_info_alternates(), passes in\n an mmap'd file. Unless the file is an exact multiple of\n the page size, it will generally be followed by NUL\n padding to the end of the page, which just works.\n\nThe easiest way to demonstrate the problem is to build with:\n\n make SANITIZE=address NO_MMAP=Nope test\n\nAny test which involves $GIT_DIR/info/alternates will fail,\nas the mmap emulation (correctly) does not add an extra NUL,\nand ASAN complains about reading past the end of the buffer.\n\nOne solution would be to teach link_alt_odb_entries() to\nrespect \"len\". But it's actually a bit tricky, since we\ndepend on unquote_c_style() under the hood, and it has no\nptr/len variant.\n\nWe could also just make a NUL-terminated copy of the input\nbytes and operate on that. But since all but one caller\nalready is passing a string, instead let's just fix that\ncaller to provide NUL-terminated input in the first place,\nby swapping out mmap for strbuf_read_file().\n\nThere's no advantage to using mmap on the alternates file.\nIt's not expected to be large (and anyway, we're copying its\ncontents into an in-memory linked list). Nor is using\ngit_open() buying us anything here, since we don't keep the\ndescriptor open for a long period of time.\n\nLet's also drop the \"len\" parameter entirely from\nlink_alt_odb_entries(), since it's completely ignored. That\nwill avoid any new callers re-introducing a similar bug.\n\nReported-by: Michael Haggerty \nSigned-off-by: Jeff King \n---\nI didn't reproduce with the mmap even-page-size thing, but I\nthink it's the same reason we didn't notice the \"git log -G\"\nread-past-mmap issues for a long time. Which makes testing\nwith ASAN and NO_MMAP an interesting combination, as it\nshould find out any similar cases (and after this, the whole\ntest suite passes with that configuration).\n\n sha1_file.c | 29 +++++++++--------------------\n 1 file changed, 9 insertions(+), 20 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5f71bbac3e..b1e4193679 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -398,7 +398,7 @@ static const char *parse_alt_odb_entry(const char *string,\n \treturn end;\n }\n \n-static void link_alt_odb_entries(const char *alt, int len, int sep,\n+static void link_alt_odb_entries(const char *alt, int sep,\n \t\t\t\t const char *relative_base, int depth)\n {\n \tstruct strbuf objdirbuf = STRBUF_INIT;\n@@ -427,28 +427,17 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \n static void read_info_alternates(const char * relative_base, int depth)\n {\n-\tchar *map;\n-\tsize_t mapsz;\n-\tstruct stat st;\n \tchar *path;\n-\tint fd;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n-\tfd = git_open(path);\n-\tfree(path);\n-\tif (fd < 0)\n-\t\treturn;\n-\tif (fstat(fd, &st) || (st.st_size == 0)) {\n-\t\tclose(fd);\n+\tif (strbuf_read_file(&buf, path, 1024) < 0) {\n+\t\tfree(path);\n \t\treturn;\n \t}\n-\tmapsz = xsize_t(st.st_size);\n-\tmap = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tclose(fd);\n \n-\tlink_alt_odb_entries(map, mapsz, '\\n', relative_base, depth);\n-\n-\tmunmap(map, mapsz);\n+\tlink_alt_odb_entries(buf.buf, '\\n', relative_base, depth);\n+\tstrbuf_release(&buf);\n }\n \n struct alternate_object_database *alloc_alt_odb(const char *dir)\n@@ -503,7 +492,7 @@ void add_to_alternates_file(const char *reference)\n \t\tif (commit_lock_file(lock))\n \t\t\tdie_errno(\"unable to move new alternates file into place\");\n \t\tif (alt_odb_tail)\n-\t\t\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+\t\t\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n \t}\n \tfree(alts);\n }\n@@ -516,7 +505,7 @@ void add_to_alternates_memory(const char *reference)\n \t */\n \tprepare_alt_odb();\n \n-\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n }\n \n /*\n@@ -619,7 +608,7 @@ void prepare_alt_odb(void)\n \tif (!alt) alt = \"\";\n \n \talt_odb_tail = &alt_odb_list;\n-\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n+\tlink_alt_odb_entries(alt, PATH_SEP, NULL, 0);\n \n \tread_info_alternates(get_object_directory(), 0);\n }\n-- \n2.14.1.1014.g252e627ae0\n\n"},{"id":"328301","messageId":"20170918155519.llmetd2ylebzokrl@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170918155059.54f7z6cnrl47f5el@sigill.intra.peff.net","subject":"[PATCH 2/2] read_info_alternates: warn on non-trivial errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-18T15:55:19Z","receivedAt":"2017-09-18T15:55:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fail to open $GIT_DIR/info/alternates, we silently\nassume there are no alternates. This is the right thing to\ndo for ENOENT, but not for other errors.\n\nA hard error is probably overkill here. If we fail to read\nan alternates file then either we'll complete our operation\nanyway, or we'll fail to find some needed object. Either\nway, a warning is good idea. And we already have a helper\nfunction to handle this pattern; let's just call\nwarn_on_fopen_error().\n\nNote that technically the errno from strbuf_read_file()\nmight be from a read() error, not open(). But since read()\nwould never return ENOENT or ENOTDIR, and since it produces\na generic \"unable to access\" error, it's suitable for\nhandling errors from either.\n\nSigned-off-by: Jeff King \n---\nI'm pretty comfortable with the rationale in the final paragraph. But if\nwe're concerned that warn_on_fopen_errors() might change, we can swap\nout strbuf_read_file() for fopen()+strbuf_read().\n\n sha1_file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b1e4193679..9cec326298 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -432,6 +432,7 @@ static void read_info_alternates(const char * relative_base, int depth)\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n \tif (strbuf_read_file(&buf, path, 1024) < 0) {\n+\t\twarn_on_fopen_errors(path);\n \t\tfree(path);\n \t\treturn;\n \t}\n-- \n2.14.1.1014.g252e627ae0\n"},{"id":"328325","messageId":"xmqqvakfsg75.fsf@gitster.mtv.corp.google.com","threadId":"46781","inReplyTo":"20170918155429.hgql4ngayun5nbyd@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] read_info_alternates: read contents into strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-19T00:08:30Z","receivedAt":"2017-09-19T00:08:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We could also just make a NUL-terminated copy of the input\n> bytes and operate on that. But since all but one caller\n> already is passing a string, instead let's just fix that\n> caller to provide NUL-terminated input in the first place,\n> by swapping out mmap for strbuf_read_file().\n> ...\n> Let's also drop the \"len\" parameter entirely from\n> link_alt_odb_entries(), since it's completely ignored. That\n> will avoid any new callers re-introducing a similar bug.\n\nBoth design decisions make perfect sense to me.\n\n> sha1_file.c | 29 +++++++++--------------------\n> 1 file changed, 9 insertions(+), 20 deletions(-)\n\nAnd diffstat also agrees that it is a good change ;-)\n"},{"id":"328332","messageId":"20170919023603.GB175206@aiede.mtv.corp.google.com","threadId":"46781","inReplyTo":"20170918155059.54f7z6cnrl47f5el@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fix read past end of array in alternates files","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-19T02:36:03Z","receivedAt":"2017-09-19T02:36:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This series fixes a regression in v2.11.1 where we might read past the\n> end of an mmap'd buffer. It was introduced in cf3c635210,\n\nThe above information is super helpful. Can it go in one of the commit\nmessages?\n\n> but I didn't\n> base the patch on there, for a few reasons:\n>\n> 1. There's a trivial conflict when merging up (because of\n> git_open_noatime() becoming just git_open() in the inerim).\n>\n> 2. The reproduction advice relies on our SANITIZE Makefile knob, which\n> didn't exist back then.\n>\n> 3. The second patch does not apply there because we don't have\n> warn_on_fopen_errors(). Though admittedly it could be applied\n> separately after merging up; it's just a clean-up on top.\n\nEven this part could go in a commit message, but it's fine for it not\nto.\n\nThanks,\nJonathan\n"},{"id":"328333","messageId":"20170919024253.GC175206@aiede.mtv.corp.google.com","threadId":"46781","inReplyTo":"20170918155429.hgql4ngayun5nbyd@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] read_info_alternates: read contents into strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-19T02:42:53Z","receivedAt":"2017-09-19T02:43:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Reported-by: Michael Haggerty \n> Signed-off-by: Jeff King \n> ---\n> sha1_file.c | 29 +++++++++--------------------\n> 1 file changed, 9 insertions(+), 20 deletions(-)\n\nThanks for tracking it down.\n\nReviewed-by: Jonathan Nieder \n\n[...]\n> +++ b/sha1_file.c\n[...]\n> @@ -427,28 +427,17 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n> \n> static void read_info_alternates(const char * relative_base, int depth)\n> {\n> -\tchar *map;\n> -\tsize_t mapsz;\n> -\tstruct stat st;\n> \tchar *path;\n> -\tint fd;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \n> \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n> -\tfd = git_open(path);\n> -\tfree(path);\n> -\tif (fd < 0)\n> -\t\treturn;\n> -\tif (fstat(fd, &st) || (st.st_size == 0)) {\n> -\t\tclose(fd);\n> +\tif (strbuf_read_file(&buf, path, 1024) < 0) {\n> +\t\tfree(path);\n> \t\treturn;\n\nstrbuf_read_file is careful to release buf on failure, so this doesn't\nleak (but it's a bit subtle).\n\nWhat happened to the !st.st_size case? Is it eliminated for\nsimplicity?\n\nThe rest looks good.\n\nThanks,\nJonathan\n"},{"id":"328334","messageId":"20170919024623.GD175206@aiede.mtv.corp.google.com","threadId":"46781","inReplyTo":"20170918155519.llmetd2ylebzokrl@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] read_info_alternates: warn on non-trivial errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-19T02:46:24Z","receivedAt":"2017-09-19T02:46:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> When we fail to open $GIT_DIR/info/alternates, we silently\n> assume there are no alternates. This is the right thing to\n> do for ENOENT, but not for other errors.\n>\n> A hard error is probably overkill here. If we fail to read\n> an alternates file then either we'll complete our operation\n> anyway, or we'll fail to find some needed object. Either\n> way, a warning is good idea. And we already have a helper\n> function to handle this pattern; let's just call\n> warn_on_fopen_error().\n\nI think I prefer a hard error. What kind of cases are you imagining\nwhere it would be better to warn?\n\nE.g. for EIO, erroring out so that the user can try again seems better\nthan hoping that the application will be able to cope with the more\nsubtle error that comes from discovering some objects are missing.\n\nFor EACCES, I can see how it makes sense to warn and move on, but no\nother errors like that are occuring to me.\n\nThoughts?\n\nThanks,\nJonathan\n\n> Note that technically the errno from strbuf_read_file()\n> might be from a read() error, not open(). But since read()\n> would never return ENOENT or ENOTDIR, and since it produces\n> a generic \"unable to access\" error, it's suitable for\n> handling errors from either.\n"},{"id":"328342","messageId":"20170919045706.evw2l37hd4r2czra@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170919023603.GB175206@aiede.mtv.corp.google.com","subject":"Re: [PATCH 0/2] fix read past end of array in alternates files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T04:57:07Z","receivedAt":"2017-09-19T04:57:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 18, 2017 at 07:36:03PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > This series fixes a regression in v2.11.1 where we might read past the\n> > end of an mmap'd buffer. It was introduced in cf3c635210,\n> \n> The above information is super helpful. Can it go in one of the commit\n> messages?\n\nEr, didn't I?\n\n> > base the patch on there, for a few reasons:\n> >\n> > 1. There's a trivial conflict when merging up (because of\n> > git_open_noatime() becoming just git_open() in the inerim).\n> >\n> > 2. The reproduction advice relies on our SANITIZE Makefile knob, which\n> > didn't exist back then.\n> >\n> > 3. The second patch does not apply there because we don't have\n> > warn_on_fopen_errors(). Though admittedly it could be applied\n> > separately after merging up; it's just a clean-up on top.\n> \n> Even this part could go in a commit message, but it's fine for it not\n> to.\n\nIMHO this kind of meta information doesn't belong in the commit message.\nIt's useful to the maintainer to know where to apply the patch, but I\ndon't think it helps somebody who is reading \"git log\" output.\n\n-Peff\n"},{"id":"328343","messageId":"20170919050316.nfbk6wckgdtmzvvd@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170919024253.GC175206@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] read_info_alternates: read contents into strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T05:03:17Z","receivedAt":"2017-09-19T05:03:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 18, 2017 at 07:42:53PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Reported-by: Michael Haggerty \n> > Signed-off-by: Jeff King \n> > ---\n> > sha1_file.c | 29 +++++++++--------------------\n> > 1 file changed, 9 insertions(+), 20 deletions(-)\n> \n> Thanks for tracking it down.\n\nTo be fair, Michael did most of the work in identifying and bisecting\nthe bug. He even wrote a very similar patch in parallel; I just swooped\nin at the end.\n\n> > \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n> > -\tfd = git_open(path);\n> > -\tfree(path);\n> > -\tif (fd < 0)\n> > -\t\treturn;\n> > -\tif (fstat(fd, &st) || (st.st_size == 0)) {\n> > -\t\tclose(fd);\n> > +\tif (strbuf_read_file(&buf, path, 1024) < 0) {\n> > +\t\tfree(path);\n> > \t\treturn;\n> \n> strbuf_read_file is careful to release buf on failure, so this doesn't\n> leak (but it's a bit subtle).\n\nYep. I didn't think it was worth calling out with a comment since the\n\"don't allocate on failure\" pattern is common to most of the strbuf\nfunctions.\n\n> What happened to the !st.st_size case? Is it eliminated for\n> simplicity?\n\nGood question, and the answer is yes. Obviously we can bail early on an\nempty file, but I don't think there's any reason to complicate the code\nwith it (the original seems to come from d5a63b9983 (Alternate object\npool mechanism updates., 2005-08-14), where it avoided a corner case\nthat has long since been deleted.\n\n-Peff\n"},{"id":"328344","messageId":"20170919051505.qdfycovqlcqa6n5c@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170919024623.GD175206@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] read_info_alternates: warn on non-trivial errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T05:15:05Z","receivedAt":"2017-09-19T05:15:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 18, 2017 at 07:46:24PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > When we fail to open $GIT_DIR/info/alternates, we silently\n> > assume there are no alternates. This is the right thing to\n> > do for ENOENT, but not for other errors.\n> >\n> > A hard error is probably overkill here. If we fail to read\n> > an alternates file then either we'll complete our operation\n> > anyway, or we'll fail to find some needed object. Either\n> > way, a warning is good idea. And we already have a helper\n> > function to handle this pattern; let's just call\n> > warn_on_fopen_error().\n> \n> I think I prefer a hard error. What kind of cases are you imagining\n> where it would be better to warn?\n> \n> E.g. for EIO, erroring out so that the user can try again seems better\n> than hoping that the application will be able to cope with the more\n> subtle error that comes from discovering some objects are missing.\n> \n> For EACCES, I can see how it makes sense to warn and move on, but no\n> other errors like that are occuring to me.\n> \n> Thoughts?\n\nYes, EACCES is exactly the example that came to mind. But most\nimportantly, we don't know at this point whether the alternate is even\nrelevant to the current operation. So calling die() here would make some\ncases fail that would otherwise succeed. That's usually not a big deal,\nbut we've had cases where being lazy about dying helps people use git\nitself to help repair the case.\n\nAdmittedly most of those chicken-and-egg problems are centered around\ngit-config (e.g., using \"git config --edit\" to repair broken config), so\nit's perhaps less important here. But it seems like a reasonable\nprinciple to follow in general.\n\nIf there's a compelling reason to die hard, I'd reconsider it. But I\ncan't really think of one (if we were _writing_ a new alternates file\nand encountered a read error of the old data we're copying, then I think\nit would be very important to bail before claiming a successful write.\nBut that's not what's happening here).\n\n-Peff\n"},{"id":"328414","messageId":"20170919194044.3prgaxd4sqandy75@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170918155059.54f7z6cnrl47f5el@sigill.intra.peff.net","subject":"[PATCH v2 0/2] fix read past end of array in alternates files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T19:40:44Z","receivedAt":"2017-09-19T19:40:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 18, 2017 at 11:51:00AM -0400, Jeff King wrote:\n\n> This series fixes a regression in v2.11.1 where we might read past the\n> end of an mmap'd buffer. It was introduced in cf3c635210, but I didn't\n> base the patch on there, for a few reasons:\n\nHere's a v2 that fixes a minor leak in the first patch (I carefully\nremembered to free() the path buffer on the error path, but forgot to do\nit in the normal one. Oops).\n\nI also tried to address Jonathan's \"should this be in the commit\nmessage\" comment. The information above _is_ in there, but maybe putting\nit at the top as a sort of tl;dr makes it easier to find?\n\nThe second patch is unchanged.\n\nJunio, I see you ended up back-porting it to v2.11. Would you prefer me\nto have done it that way in the first place? I was trying to reduce your\nwork by basing it on \"maint\" (figuring that we wouldn't bother making a\nv2.11.x release anyway, and that skips you having to apply the second\npatch separately after the merge).\n\n [1/2]: read_info_alternates: read contents into strbuf\n [2/2]: read_info_alternates: warn on non-trivial errors\n\n sha1_file.c | 31 +++++++++++--------------------\n 1 file changed, 11 insertions(+), 20 deletions(-)\n\n-Peff\n"},{"id":"328415","messageId":"20170919194106.x43fxrgfft7jhdin@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170919194044.3prgaxd4sqandy75@sigill.intra.peff.net","subject":"[PATCH v2 1/2] read_info_alternates: read contents into strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T19:41:07Z","receivedAt":"2017-09-19T19:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch fixes a regression in v2.11.1 where we might read\npast the end of an mmap'd buffer. It was introduced in\ncf3c635210.\n\nThe link_alt_odb_entries() function has always taken a\nptr/len pair as input. Until cf3c635210 (alternates: accept\ndouble-quoted paths, 2016-12-12), we made a copy of those\nbytes in a string. But after that commit, we switched to\nparsing the input left-to-right, and we ignore \"len\"\ntotally, instead reading until we hit a NUL.\n\nThis has mostly gone unnoticed for a few reasons:\n\n 1. All but one caller passes a NUL-terminated string, with\n \"len\" pointing to the NUL.\n\n 2. The remaining caller, read_info_alternates(), passes in\n an mmap'd file. Unless the file is an exact multiple of\n the page size, it will generally be followed by NUL\n padding to the end of the page, which just works.\n\nThe easiest way to demonstrate the problem is to build with:\n\n make SANITIZE=address NO_MMAP=Nope test\n\nAny test which involves $GIT_DIR/info/alternates will fail,\nas the mmap emulation (correctly) does not add an extra NUL,\nand ASAN complains about reading past the end of the buffer.\n\nOne solution would be to teach link_alt_odb_entries() to\nrespect \"len\". But it's actually a bit tricky, since we\ndepend on unquote_c_style() under the hood, and it has no\nptr/len variant.\n\nWe could also just make a NUL-terminated copy of the input\nbytes and operate on that. But since all but one caller\nalready is passing a string, instead let's just fix that\ncaller to provide NUL-terminated input in the first place,\nby swapping out mmap for strbuf_read_file().\n\nThere's no advantage to using mmap on the alternates file.\nIt's not expected to be large (and anyway, we're copying its\ncontents into an in-memory linked list). Nor is using\ngit_open() buying us anything here, since we don't keep the\ndescriptor open for a long period of time.\n\nLet's also drop the \"len\" parameter entirely from\nlink_alt_odb_entries(), since it's completely ignored. That\nwill avoid any new callers re-introducing a similar bug.\n\nReported-by: Michael Haggerty \nSigned-off-by: Jeff King \n---\n sha1_file.c | 30 ++++++++++--------------------\n 1 file changed, 10 insertions(+), 20 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b4a67bb838..b682b7ec06 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -398,7 +398,7 @@ static const char *parse_alt_odb_entry(const char *string,\n \treturn end;\n }\n \n-static void link_alt_odb_entries(const char *alt, int len, int sep,\n+static void link_alt_odb_entries(const char *alt, int sep,\n \t\t\t\t const char *relative_base, int depth)\n {\n \tstruct strbuf objdirbuf = STRBUF_INIT;\n@@ -427,28 +427,18 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \n static void read_info_alternates(const char * relative_base, int depth)\n {\n-\tchar *map;\n-\tsize_t mapsz;\n-\tstruct stat st;\n \tchar *path;\n-\tint fd;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n-\tfd = git_open(path);\n-\tfree(path);\n-\tif (fd < 0)\n-\t\treturn;\n-\tif (fstat(fd, &st) || (st.st_size == 0)) {\n-\t\tclose(fd);\n+\tif (strbuf_read_file(&buf, path, 1024) < 0) {\n+\t\tfree(path);\n \t\treturn;\n \t}\n-\tmapsz = xsize_t(st.st_size);\n-\tmap = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tclose(fd);\n-\n-\tlink_alt_odb_entries(map, mapsz, '\\n', relative_base, depth);\n \n-\tmunmap(map, mapsz);\n+\tlink_alt_odb_entries(buf.buf, '\\n', relative_base, depth);\n+\tstrbuf_release(&buf);\n+\tfree(path);\n }\n \n struct alternate_object_database *alloc_alt_odb(const char *dir)\n@@ -503,7 +493,7 @@ void add_to_alternates_file(const char *reference)\n \t\tif (commit_lock_file(lock))\n \t\t\tdie_errno(\"unable to move new alternates file into place\");\n \t\tif (alt_odb_tail)\n-\t\t\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+\t\t\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n \t}\n \tfree(alts);\n }\n@@ -516,7 +506,7 @@ void add_to_alternates_memory(const char *reference)\n \t */\n \tprepare_alt_odb();\n \n-\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+\tlink_alt_odb_entries(reference, '\\n', NULL, 0);\n }\n \n /*\n@@ -619,7 +609,7 @@ void prepare_alt_odb(void)\n \tif (!alt) alt = \"\";\n \n \talt_odb_tail = &alt_odb_list;\n-\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n+\tlink_alt_odb_entries(alt, PATH_SEP, NULL, 0);\n \n \tread_info_alternates(get_object_directory(), 0);\n }\n-- \n2.14.1.1014.g252e627ae0\n\n"},{"id":"328416","messageId":"20170919194110.ngmgrlph3ffadvoj@sigill.intra.peff.net","threadId":"46781","inReplyTo":"20170919194044.3prgaxd4sqandy75@sigill.intra.peff.net","subject":"[PATCH v2 2/2] read_info_alternates: warn on non-trivial errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T19:41:10Z","receivedAt":"2017-09-19T19:41:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fail to open $GIT_DIR/info/alternates, we silently\nassume there are no alternates. This is the right thing to\ndo for ENOENT, but not for other errors.\n\nA hard error is probably overkill here. If we fail to read\nan alternates file then either we'll complete our operation\nanyway, or we'll fail to find some needed object. Either\nway, a warning is good idea. And we already have a helper\nfunction to handle this pattern; let's just call\nwarn_on_fopen_error().\n\nNote that technically the errno from strbuf_read_file()\nmight be from a read() error, not open(). But since read()\nwould never return ENOENT or ENOTDIR, and since it produces\na generic \"unable to access\" error, it's suitable for\nhandling errors from either.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b682b7ec06..1477ea7b50 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -432,6 +432,7 @@ static void read_info_alternates(const char * relative_base, int depth)\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n \tif (strbuf_read_file(&buf, path, 1024) < 0) {\n+\t\twarn_on_fopen_errors(path);\n \t\tfree(path);\n \t\treturn;\n \t}\n-- \n2.14.1.1014.g252e627ae0\n"},{"id":"328440","messageId":"xmqqmv5qozq7.fsf@gitster.mtv.corp.google.com","threadId":"46781","inReplyTo":"20170919194044.3prgaxd4sqandy75@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/2] fix read past end of array in alternates files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T02:44:48Z","receivedAt":"2017-09-20T02:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 18, 2017 at 11:51:00AM -0400, Jeff King wrote:\n>\n>> This series fixes a regression in v2.11.1 where we might read past the\n>> end of an mmap'd buffer. It was introduced in cf3c635210, but I didn't\n>> base the patch on there, for a few reasons:\n>\n> Here's a v2 that fixes a minor leak in the first patch (I carefully\n> remembered to free() the path buffer on the error path, but forgot to do\n> it in the normal one. Oops).\n\nThanks.\n\n> I also tried to address Jonathan's \"should this be in the commit\n> message\" comment. The information above _is_ in there, but maybe putting\n> it at the top as a sort of tl;dr makes it easier to find?\n\nProbably.\n\n> The second patch is unchanged.\n>\n> Junio, I see you ended up back-porting it to v2.11. Would you prefer me\n> to have done it that way in the first place? I was trying to reduce your\n> work by basing it on \"maint\" (figuring that we wouldn't bother making a\n> v2.11.x release anyway, and that skips you having to apply the second\n> patch separately after the merge).\n\nUpon seeing that this dated back to 2.11, because I am lazy and do\nnot assess how much the fix needs to go to older tracks when I am\nqueuing (remember: my attention span during patch queueing is\nmeasured in minutes, as people send changes to different areas), I\ntend to first try to see what's the oldest maintenance track we can\npractically apply the patch to. It turned out that the conflict\nresolution to apply on maint-2.11 wasn't that painful, so I took the\nlazy route all the way---the real fix on the oldest, even though I\ndo not know (because I refused to think and decide due to laziness)\nif a next v2.11.x release is necessary, followed by a nice-to-have\nwarning that uses newer features on the maintenance track. That\nway, when we decide that the fix won't be a big deal to require a\nnew v2.11.x, but it is nice to have in v2.13.x, we could merge the\nfirst one, without having to cherry-pick.\n\nAll of the above is part of how the daily maintainer workflow goes,\nand there is no strong preference on my side, if the original is on\nthe theoretically oldest (i.e. maint-2.11) or on the oldest\npractical (i.e. maint), as long as the conflicts are not too\npainful.\n\n"}]}