{"thread":{"id":"46905","subject":"[PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest","startedAt":"2017-10-05T05:59:58Z","lastAt":"2017-10-10T01:34:15Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"329725","messageId":"20171005055952.t5ef7hyolyevoj3d@sigill.intra.peff.net","threadId":"46905","inReplyTo":null,"subject":"[PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-05T05:59:52Z","receivedAt":"2017-10-05T05:59:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a caller of sha1_object_info_extended() sets the\n\"contentp\" field in object_info, we call unpack_sha1_rest()\nbut do not check whether it signaled an error.\n\nThis causes two problems:\n\n 1. We pass back NULL to the caller via the contentp field,\n but the function returns \"0\" for success. A caller\n might reasonably expect after a successful return that\n it can access contentp without a NULL check and\n segfault.\n\n As it happens, this is impossible to trigger in the\n current code. There is exactly one caller which uses\n contentp, read_object(). And the only thing it does\n after a successful call is to return the content\n pointer to its caller, using NULL as a sentinel for\n errors. So in effect it converts the success code from\n sha1_object_info_extended() back into an error!\n\n But this is still worth addressing avoid problems for\n future users of \"contentp\".\n\n 2. Callers of unpack_sha1_rest() are expected to close the\n zlib stream themselves on error. Which means that we're\n leaking the stream.\n\nThe problem in (1) comes from from c84a1f3ed4 (sha1_file:\nrefactor read_object, 2017-06-21), which added the contentp\nfield. Before that, we called unpack_sha1_rest() via\nunpack_sha1_file(), which directly used the NULL to signal\nan error.\n\nBut note that the leak in (2) is actually older than that.\nThe original unpack_sha1_file() directly returned the result\nof unpack_sha1_rest() to its caller, when it should have\nbeen closing the zlib stream itself on error.\n\nSigned-off-by: Jeff King \n---\n sha1_file.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09ad64ce55..10c3a0083d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1124,10 +1124,14 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n \t} else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)\n \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n \n-\tif (status >= 0 && oi->contentp)\n+\tif (status >= 0 && oi->contentp) {\n \t\t*oi->contentp = unpack_sha1_rest(&stream, hdr,\n \t\t\t\t\t\t *oi->sizep, sha1);\n-\telse\n+\t\tif (!*oi->contentp) {\n+\t\t\tgit_inflate_end(&stream);\n+\t\t\tstatus = -1;\n+\t\t}\n+\t} else\n \t\tgit_inflate_end(&stream);\n \n \tmunmap(map, mapsize);\n-- \n2.14.2.1117.g65a3442612\n"},{"id":"329842","messageId":"xmqqr2ugykl2.fsf@gitster.mtv.corp.google.com","threadId":"46905","inReplyTo":"20171005055952.t5ef7hyolyevoj3d@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T04:19:21Z","receivedAt":"2017-10-06T04:19:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When a caller of sha1_object_info_extended() sets the\n> \"contentp\" field in object_info, we call unpack_sha1_rest()\n> but do not check whether it signaled an error.\n>\n> This causes two problems:\n>\n> 1. We pass back NULL to the caller via the contentp field,\n> but the function returns \"0\" for success. A caller\n> might reasonably expect after a successful return that\n> it can access contentp without a NULL check and\n> segfault.\n>\n> As it happens, this is impossible to trigger in the\n> current code. There is exactly one caller which uses\n> contentp, read_object(). And the only thing it does\n> after a successful call is to return the content\n> pointer to its caller, using NULL as a sentinel for\n> errors. So in effect it converts the success code from\n> sha1_object_info_extended() back into an error!\n>\n> But this is still worth addressing avoid problems for\n> future users of \"contentp\".\n>\n> 2. Callers of unpack_sha1_rest() are expected to close the\n> zlib stream themselves on error. Which means that we're\n> leaking the stream.\n>\n> The problem in (1) comes from from c84a1f3ed4 (sha1_file:\n> refactor read_object, 2017-06-21), which added the contentp\n> field. Before that, we called unpack_sha1_rest() via\n> unpack_sha1_file(), which directly used the NULL to signal\n> an error.\n>\n> But note that the leak in (2) is actually older than that.\n> The original unpack_sha1_file() directly returned the result\n> of unpack_sha1_rest() to its caller, when it should have\n> been closing the zlib stream itself on error.\n>\n> Signed-off-by: Jeff King \n> ---\n\nObviously correct. (2) is as old as Git itself; it eventually\nblames down to e83c5163 (\"Initial revision of \"git\", the information\nmanager from hell\", 2005-04-07), where read-cache.c::unpack_sha1_file()\nliberally returns NULL without cleaning up the zstream.\n\nThanks.\n\n> sha1_file.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 09ad64ce55..10c3a0083d 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1124,10 +1124,14 @@ static int sha1_loose_object_info(const unsigned char *sha1,\n> \t} else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)\n> \t\tstatus = error(\"unable to parse %s header\", sha1_to_hex(sha1));\n> \n> -\tif (status >= 0 && oi->contentp)\n> +\tif (status >= 0 && oi->contentp) {\n> \t\t*oi->contentp = unpack_sha1_rest(&stream, hdr,\n> \t\t\t\t\t\t *oi->sizep, sha1);\n> -\telse\n> +\t\tif (!*oi->contentp) {\n> +\t\t\tgit_inflate_end(&stream);\n> +\t\t\tstatus = -1;\n> +\t\t}\n> +\t} else\n> \t\tgit_inflate_end(&stream);\n> \n> \tmunmap(map, mapsize);\n"},{"id":"329844","messageId":"20171006043008.2s6pg4aurwledf3s@sigill.intra.peff.net","threadId":"46905","inReplyTo":"xmqqr2ugykl2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T04:30:08Z","receivedAt":"2017-10-06T04:30:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 01:19:21PM +0900, Junio C Hamano wrote:\n\n> > But note that the leak in (2) is actually older than that.\n> > The original unpack_sha1_file() directly returned the result\n> > of unpack_sha1_rest() to its caller, when it should have\n> > been closing the zlib stream itself on error.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> \n> Obviously correct. (2) is as old as Git itself; it eventually\n> blames down to e83c5163 (\"Initial revision of \"git\", the information\n> manager from hell\", 2005-04-07), where read-cache.c::unpack_sha1_file()\n> liberally returns NULL without cleaning up the zstream.\n\nThanks, I as too lazy to dig down further, but I'm always interested to\nsee the roots of these things (especially \"bug in the original\" versus\n\"introduced by a careless refactor\").\n\nI have a feeling that the world would be a better place if\nunpack_sha1_rest() just always promised to close the zstream, since no\ncallers seem to want to look at it in the error case. But I wanted to go\nfor the minimal fix first.\n\n-Peff\n"},{"id":"329846","messageId":"20171006043847.hja2yjlvatckgy4d@sigill.intra.peff.net","threadId":"46905","inReplyTo":"20171006043008.2s6pg4aurwledf3s@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T04:38:47Z","receivedAt":"2017-10-06T04:38:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 12:30:08AM -0400, Jeff King wrote:\n\n> On Fri, Oct 06, 2017 at 01:19:21PM +0900, Junio C Hamano wrote:\n> \n> > > But note that the leak in (2) is actually older than that.\n> > > The original unpack_sha1_file() directly returned the result\n> > > of unpack_sha1_rest() to its caller, when it should have\n> > > been closing the zlib stream itself on error.\n> > >\n> > > Signed-off-by: Jeff King \n> > > ---\n> > \n> > Obviously correct. (2) is as old as Git itself; it eventually\n> > blames down to e83c5163 (\"Initial revision of \"git\", the information\n> > manager from hell\", 2005-04-07), where read-cache.c::unpack_sha1_file()\n> > liberally returns NULL without cleaning up the zstream.\n> \n> Thanks, I as too lazy to dig down further, but I'm always interested to\n> see the roots of these things (especially \"bug in the original\" versus\n> \"introduced by a careless refactor\").\n> \n> I have a feeling that the world would be a better place if\n> unpack_sha1_rest() just always promised to close the zstream, since no\n> callers seem to want to look at it in the error case. But I wanted to go\n> for the minimal fix first.\n\nActually, there are only two callers left these days. One of them leaks,\nand the other immediately closes the zstream. So something like:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 09ad64ce55..cea003d182 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -978,10 +978,10 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s\n \t\twhile (status == Z_OK)\n \t\t\tstatus = git_inflate(stream, Z_FINISH);\n \t}\n-\tif (status == Z_STREAM_END && !stream->avail_in) {\n-\t\tgit_inflate_end(stream);\n+\tgit_inflate_end(stream);\n+\n+\tif (status == Z_STREAM_END && !stream->avail_in)\n \t\treturn buf;\n-\t}\n \n \tif (status < 0)\n \t\terror(\"corrupt loose object '%s'\", sha1_to_hex(sha1));\n@@ -2107,7 +2107,6 @@ int read_loose_object(const char *path,\n \t\t*contents = unpack_sha1_rest(&stream, hdr, *size, expected_sha1);\n \t\tif (!*contents) {\n \t\t\terror(\"unable to unpack contents of %s\", path);\n-\t\t\tgit_inflate_end(&stream);\n \t\t\tgoto out;\n \t\t}\n \t\tif (check_sha1_signature(expected_sha1, *contents,\n\nseems reasonable. Doing it that (with my other patch on top) splits the\nleak-fix and the not-yet-a-bug-but-confusing-error-return problems into\ntwo separate patches.\n\nI dunno. There aren't that many callers of unpack_sha1_rest(), so it may\nnot matter that much, but while we're here...\n\n-Peff\n"},{"id":"330081","messageId":"xmqq376rrdkm.fsf@gitster.mtv.corp.google.com","threadId":"46905","inReplyTo":"20171006043847.hja2yjlvatckgy4d@sigill.intra.peff.net","subject":"Re: [PATCH] sha1_loose_object_info: handle errors from unpack_sha1_rest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T01:34:01Z","receivedAt":"2017-10-10T01:34:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Actually, there are only two callers left these days. One of them leaks,\n> and the other immediately closes the zstream. So something like:\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 09ad64ce55..cea003d182 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -978,10 +978,10 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s\n> \t\twhile (status == Z_OK)\n> \t\t\tstatus = git_inflate(stream, Z_FINISH);\n> \t}\n> -\tif (status == Z_STREAM_END && !stream->avail_in) {\n> -\t\tgit_inflate_end(stream);\n> +\tgit_inflate_end(stream);\n> +\n> +\tif (status == Z_STREAM_END && !stream->avail_in)\n> \t\treturn buf;\n> -\t}\n> \n> \tif (status < 0)\n> \t\terror(\"corrupt loose object '%s'\", sha1_to_hex(sha1));\n> @@ -2107,7 +2107,6 @@ int read_loose_object(const char *path,\n> \t\t*contents = unpack_sha1_rest(&stream, hdr, *size, expected_sha1);\n> \t\tif (!*contents) {\n> \t\t\terror(\"unable to unpack contents of %s\", path);\n> -\t\t\tgit_inflate_end(&stream);\n> \t\t\tgoto out;\n> \t\t}\n> \t\tif (check_sha1_signature(expected_sha1, *contents,\n>\n> seems reasonable. Doing it that (with my other patch on top) splits the\n> leak-fix and the not-yet-a-bug-but-confusing-error-return problems into\n> two separate patches.\n>\n> I dunno. There aren't that many callers of unpack_sha1_rest(), so it may\n> not matter that much, but while we're here...\n\nYeah, I agree that it is a reasonable thing to do.\n"}]}