{"thread":{"id":"33198","subject":"[PATCH 0/3] fix unparsed object access in upload-pack","startedAt":"2013-03-16T10:24:28Z","lastAt":"2013-03-17T16:38:59Z","messageCount":9,"participants":["Jeff King","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"211449","messageId":"20130316102428.GA29358@sigill.intra.peff.net","threadId":"33198","inReplyTo":null,"subject":"[PATCH 0/3] fix unparsed object access in upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T10:24:28Z","receivedAt":"2013-03-16T10:24:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes the issue I mentioned recently with upload-pack, where\nwe might feed unparsed objects to the revision parser. The bug is in\n435c833 (the tip of the jk/peel-ref topic), which is in v1.8.1 and up.\n\nThe fix should go to maint.  The bug breaks shallow clones from\nrepositories with packed refs, though the effects range from working OK\nto sending too many objects to calling die() in the revision traversal\nmachinery. Given the size of the breakage (I had several bug reports\nwithin a few hours of deploying v1.8.1.5 on github.com), and the fact\nthat the bug was introduced only in v1.8.1, it may make sense to roll a\nv1.8.1.6 in addition to putting it on the 1.8.2 maint track.\n\n  [1/3]: upload-pack: drop lookup-before-parse optimization\n\n    This is just a cleanup, and is not necessary for the fix.\n\n  [2/3]: upload-pack: make sure \"want\" objects are parsed\n\n    This is the fix itself, which can stand alone from the other two\n    patches, both semantically and textually.\n\n  [3/3]: upload-pack: load non-tip \"want\" objects from disk\n\n    While investigating the bug, I found some weirdness around the\n    stateless-rpc check_non_tip code. As far as I can tell, that code\n    never actually gets triggered. It's not too surprising that we\n    wouldn't have noticed, because it is about falling back due to a\n    race condition. But please sanity check my explanation and patch.\n\n-Peff\n"},{"id":"211450","messageId":"20130316102525.GA29533@sigill.intra.peff.net","threadId":"33198","inReplyTo":"20130316102428.GA29358@sigill.intra.peff.net","subject":"[PATCH 1/3] upload-pack: drop lookup-before-parse optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T10:25:25Z","receivedAt":"2013-03-16T10:25:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we receive a \"have\" line from the client, we want to\nload the object pointed to by the sha1. However, we are\ncareful to do:\n\n  o = lookup_object(sha1);\n  if (!o || !o->parsed)\n\t  o = parse_object(sha1);\n\nto avoid loading the object from disk if we have already\nseen it.  However, since ccdc603 (parse_object: try internal\ncache before reading object db), parse_object already does\nthis optimization internally. We can just call parse_object\ndirectly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 30146a0..6bf81aa 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -325,9 +325,7 @@ static int got_sha1(char *hex, unsigned char *sha1)\n \tif (!has_sha1_file(sha1))\n \t\treturn -1;\n \n-\to = lookup_object(sha1);\n-\tif (!(o && o->parsed))\n-\t\to = parse_object(sha1);\n+\to = parse_object(sha1);\n \tif (!o)\n \t\tdie(\"oops (%s)\", sha1_to_hex(sha1));\n \tif (o->type == OBJ_COMMIT) {\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211451","messageId":"20130316102701.GB29533@sigill.intra.peff.net","threadId":"33198","inReplyTo":"20130316102428.GA29358@sigill.intra.peff.net","subject":"[PATCH 2/3] upload-pack: make sure \"want\" objects are parsed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T10:27:01Z","receivedAt":"2013-03-16T10:27:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When upload-pack receives a \"want\" line from the client, it\nadds it to an object array. We call lookup_object to find\nthe actual object, which will only check for objects already\nin memory. This works because we are expecting to find\nobjects that we already loaded during the ref advertisement.\n\nWe use the resulting object structs for a variety of\npurposes. Some of them care only about the object flags, but\nothers care about the type of the object (e.g.,\nok_to_give_up), or even feed them to the revision parser\n(when --depth is used), which assumes that objects it\nreceives are fully parsed.\n\nOnce upon a time, this was OK; any object we loaded into\nmemory would also have been parsed. But since 435c833\n(upload-pack: use peel_ref for ref advertisements,\n2012-10-04), we try to avoid parsing objects during the ref\nadvertisement. This means that lookup_object may return an\nobject with a type of OBJ_NONE. The resulting mess depends\non the exact set of objects, but can include the revision\nparser barfing, or the shallow code sending the wrong set of\nobjects.\n\nThis patch teaches upload-pack to parse each \"want\" object\nas we receive it. We do not replace the lookup_object call\nwith parse_object, as the current code is careful not to let\njust any object appear on a \"want\" line, but rather only one\nwe have previously advertised (whereas parse_object would\nactually load any arbitrary object from disk).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5500-fetch-pack.sh | 9 +++++++++\n upload-pack.c         | 2 +-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 354d32c..d574085 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -364,6 +364,15 @@ EOF\n \ttest_cmp count7.expected count7.actual\n '\n \n+test_expect_success 'clone shallow with packed refs' '\n+\tgit pack-refs --all &&\n+\tgit clone --depth 1 --branch A \"file://$(pwd)/.\" shallow8 &&\n+\techo \"in-pack: 4\" > count8.expected &&\n+\tGIT_DIR=shallow8/.git git count-objects -v |\n+\t\tgrep \"^in-pack\" > count8.actual &&\n+\ttest_cmp count8.expected count8.actual\n+'\n+\n test_expect_success 'setup tests for the --stdin parameter' '\n \tfor head in C D E F\n \tdo\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6bf81aa..41736ec 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -639,7 +639,7 @@ static void receive_needs(void)\n \t\t\tuse_include_tag = 1;\n \n \t\to = lookup_object(sha1_buf);\n-\t\tif (!o)\n+\t\tif (!o || !parse_object(o->sha1))\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t    sha1_to_hex(sha1_buf));\n \t\tif (!(o->flags & WANTED)) {\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211452","messageId":"20130316102830.GC29533@sigill.intra.peff.net","threadId":"33198","inReplyTo":"20130316102428.GA29358@sigill.intra.peff.net","subject":"[PATCH 3/3] upload-pack: load non-tip \"want\" objects from disk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-16T10:28:30Z","receivedAt":"2013-03-16T10:28:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It is a long-time security feature that upload-pack will not\nserve any \"want\" lines that do not correspond to the tip of\none of our refs. Traditionally, this was enforced by\nchecking the objects in the in-memory hash; they should have\nbeen loaded and received the OUR_REF flag during the\nadvertisement.\n\nThe stateless-rpc mode, however, has a race condition here:\none process advertises, and another receives the want lines,\nso the refs may have changed in the interim.  To address\nthis, commit 051e400 added a new verification mode; if the\nobject is not OUR_REF, we set a \"has_non_tip\" flag, and then\nlater verify that the requested objects are reachable from\nour current tips.\n\nHowever, we still die immediately when the object is not in\nour in-memory hash, and at this point we should only have\nloaded our tip objects. So the check_non_tip code path does\nnot ever actually trigger, as any non-tip objects would\nhave already caused us to die.\n\nWe can fix that by using parse_object instead of\nlookup_object, which will load the object from disk if it\nhas not already been loaded.\n\nWe still need to check that parse_object does not return\nNULL, though, as it is possible we do not have the object\nat all. A more appropriate error message would be \"no such\nobject\" rather than \"not our ref\"; however, we do not want\nto leak information about what objects are or are not in\nthe object database, so we continue to use the same \"not\nour ref\" message that would be produced by an unreachable\nobject.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n upload-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 41736ec..948cfff 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -638,8 +638,8 @@ static void receive_needs(void)\n \t\tif (parse_feature_request(features, \"include-tag\"))\n \t\t\tuse_include_tag = 1;\n \n-\t\to = lookup_object(sha1_buf);\n-\t\tif (!o || !parse_object(o->sha1))\n+\t\to = parse_object(sha1_buf);\n+\t\tif (!o)\n \t\t\tdie(\"git upload-pack: not our ref %s\",\n \t\t\t    sha1_to_hex(sha1_buf));\n \t\tif (!(o->flags & WANTED)) {\n-- \n1.8.2.rc2.7.gef06216\n"},{"id":"211484","messageId":"7v7gl6txl3.fsf@alter.siamese.dyndns.org","threadId":"33198","inReplyTo":"20130316102428.GA29358@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fix unparsed object access in upload-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T05:16:40Z","receivedAt":"2013-03-17T05:16:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This series fixes the issue I mentioned recently with upload-pack, where\n> we might feed unparsed objects to the revision parser. The bug is in\n> 435c833 (the tip of the jk/peel-ref topic), which is in v1.8.1 and up.\n\nGood to see follow-up from a responsible contributor ;-)\n\n> ... (I had several bug reports\n> within a few hours of deploying v1.8.1.5 on github.com)\n\nNice to have a pro at the widely used site ;-)  I often wish it had\na mechanism to deploy the tip of 'master' or 'maint', or even 'next'\nfor 0.2% of its users' repositories to give us an early feedback.\n\n>   [3/3]: upload-pack: load non-tip \"want\" objects from disk\n>\n>     While investigating the bug, I found some weirdness around the\n>     stateless-rpc check_non_tip code. As far as I can tell, that code\n>     never actually gets triggered. It's not too surprising that we\n>     wouldn't have noticed, because it is about falling back due to a\n>     race condition. But please sanity check my explanation and patch.\n\nThanks. That fall-back is Shawn's doing and I suspect that nobody is\nexercising the codepath (he isn't).\n"},{"id":"211488","messageId":"20130317054039.GA16070@sigill.intra.peff.net","threadId":"33198","inReplyTo":"7v7gl6txl3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] fix unparsed object access in upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T05:40:39Z","receivedAt":"2013-03-17T05:40:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 16, 2013 at 10:16:40PM -0700, Junio C Hamano wrote:\n\n> > ... (I had several bug reports\n> > within a few hours of deploying v1.8.1.5 on github.com)\n> \n> Nice to have a pro at the widely used site ;-)  I often wish it had\n> a mechanism to deploy the tip of 'master' or 'maint', or even 'next'\n> for 0.2% of its users' repositories to give us an early feedback.\n\nWe are quite slow and conservative at deploying new git, preferring\ninstead to let the rest of the world act as our testbed. :)\n\nAs seen with this bug, though, we really do get a lot more coverage of\nweird cases due to our size. In the cases I looked at, the trigger\nseemed to be clients doing the equivalent of of \"git clone --depth=X\n--no-single-branch\". Almost nobody would do that intentionally, but\nprior to v1.7.10, we did not have --single-branch; older clients using\n--depth on a repository with multiple branches started failing clones\nalmost immediately.\n\nWe do have the capability to roll out to one or a few of our servers\n(the granularity is not 0.2%, but it is still small). I'm going to try\nto keep us more in sync with upstream git, but I don't know if I will\nget to the point of ever deploying \"master\" or \"next\", even for a small\nportion of the population. We are accumulating more hacks[1] on top of\ngit, so it is not just \"run master for an hour on this server\"; I have\nto actually merge our fork.\n\nI had been handling our hacks as patch series to be rebased on top of\nupstream git releases, but that was getting increasingly unwieldy\n(especially as people besides me work on it). Going forward, I'm going\nto treat upstream git as a vendor branch and merge in occasionally to\nget fixes.\n\nOne thing I might try is to keep a local \"next-upstream\" branch, that is\ncontinually merging what is on upstream master with our local production\nversion. Then I could graduate it to production just like any other\ntopic branch when it comes time (instead of doing a gigantic painful\nmerge when we decide to upgrade upstream git).\n\nThat would mean I could test-deploy the \"next-upstream\" branch to a few\nservers on any given day without doing a lot of work. So it might make\nsense to do it at key times, like when we are in -rc here, to help shake\nout any existing bugs before you make a release.\n\n> >   [3/3]: upload-pack: load non-tip \"want\" objects from disk\n> >\n> >     While investigating the bug, I found some weirdness around the\n> >     stateless-rpc check_non_tip code. As far as I can tell, that code\n> >     never actually gets triggered. It's not too surprising that we\n> >     wouldn't have noticed, because it is about falling back due to a\n> >     race condition. But please sanity check my explanation and patch.\n> \n> Thanks. That fall-back is Shawn's doing and I suspect that nobody is\n> exercising the codepath (he isn't).\n\nI almost wonder if we should cut it out entirely. It is definitely a\npossible race condition, but I wonder if anybody actually hits it in\npractice (and if they do, the consequence is that the fetch fails and\nneeds to be retried). As far as I can tell, the code path has never\nactually been followed, and I do not recall ever seeing a bug report or\ncomplaint about it (though perhaps it happened once, which spurred the\ninitial development?).\n\n-Peff\n"},{"id":"211495","messageId":"7vk3p6sg7l.fsf@alter.siamese.dyndns.org","threadId":"33198","inReplyTo":"20130317054039.GA16070@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fix unparsed object access in upload-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T06:17:18Z","receivedAt":"2013-03-17T06:17:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> >   [3/3]: upload-pack: load non-tip \"want\" objects from disk\n>> >\n>> >     While investigating the bug, I found some weirdness around the\n>> >     stateless-rpc check_non_tip code. As far as I can tell, that code\n>> >     never actually gets triggered. It's not too surprising that we\n>> >     wouldn't have noticed, because it is about falling back due to a\n>> >     race condition. But please sanity check my explanation and patch.\n>> \n>> Thanks. That fall-back is Shawn's doing and I suspect that nobody is\n>> exercising the codepath (he isn't).\n>\n> I almost wonder if we should cut it out entirely. It is definitely a\n> possible race condition, but I wonder if anybody actually hits it in\n> practice (and if they do, the consequence is that the fetch fails and\n> needs to be retried). As far as I can tell, the code path has never\n> actually been followed, and I do not recall ever seeing a bug report or\n> complaint about it (though perhaps it happened once, which spurred the\n> initial development?).\n\nIf you run multiple servers serving the same repository at the same\nURL with a small mirroring lag, one may observe a set of refs from\none server, that are a tad older than the other server you actually\nfetch from.  k.org may have such an arrangement, but does GitHub\nserve the same repository on multiple machines without tying the\nsame client to the same backend?\n"},{"id":"211508","messageId":"20130317084750.GA934@sigill.intra.peff.net","threadId":"33198","inReplyTo":"7vk3p6sg7l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] fix unparsed object access in upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-17T08:47:51Z","receivedAt":"2013-03-17T08:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 16, 2013 at 11:17:18PM -0700, Junio C Hamano wrote:\n\n> > I almost wonder if we should cut it out entirely. It is definitely a\n> > possible race condition, but I wonder if anybody actually hits it in\n> > practice (and if they do, the consequence is that the fetch fails and\n> > needs to be retried). As far as I can tell, the code path has never\n> > actually been followed, and I do not recall ever seeing a bug report or\n> > complaint about it (though perhaps it happened once, which spurred the\n> > initial development?).\n> \n> If you run multiple servers serving the same repository at the same\n> URL with a small mirroring lag, one may observe a set of refs from\n> one server, that are a tad older than the other server you actually\n> fetch from.  k.org may have such an arrangement, but does GitHub\n> serve the same repository on multiple machines without tying the\n> same client to the same backend?\n\nEach repository is a on a single backend host. They're redundant\ninternally (each host is actually multiple hosts), but pure-git requests\ngo to a single master for each host (though for some read-only\noperations I think we spread the load across the redundant spares). You\nmight get a separate machine during a failover event, but they share\nblock devices via DRBD, so in theory an fsync() should hit both\nmachines, and there is no lag (and you are likely to get an intermittent\nfailure in such a case, anyway, since the machine serving your git\nrequest probably died mid-packet).\n\nI thought this change was to prevent against the common race:\n\n  1. Client request stateless ref advertisement.\n\n  2. Somebody updates ref.\n\n  3. Client requests \"want\" objects based on old advertisement.\n\nand I think it does solve that (assuming step 2 is not a rewind). The\nimportant thing is that time always moves forward.\n\nBut if you are talking about mirror lag, time can move in either\ndirection. Imagine you have two machines, A and B, and A is missing an\nupdate to B. If you hit A first, then B, it is the same as the update\nsequence above. The patch helps. But if you hit B first, then A, you\nwill ask it for objects it has not yet received, and we must fail.\n\nSo I think any such mirroring setup would want to try very hard to make\nsure you hit the same machine both times anyway, regardless of this\npatch.\n\nI'm fine to leave it. I was just questioning its utility since AFAICT,\nit has never worked and nobody has cared. It's not too much code,\nthough, and it is only run when we hit the race, so I don't think it is\nhurting anything.\n\n-Peff\n"},{"id":"211521","messageId":"5145F1A3.1030906@lsrfire.ath.cx","threadId":"33198","inReplyTo":"20130317054039.GA16070@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] fix unparsed object access in upload-pack","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-03-17T16:38:59Z","receivedAt":"2013-03-17T16:38:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.03.2013 06:40, schrieb Jeff King:\n> We do have the capability to roll out to one or a few of our servers\n> (the granularity is not 0.2%, but it is still small). I'm going to try\n> to keep us more in sync with upstream git, but I don't know if I will\n> get to the point of ever deploying \"master\" or \"next\", even for a small\n> portion of the population. We are accumulating more hacks[1] on top of\n> git, so it is not just \"run master for an hour on this server\"; I have\n> to actually merge our fork.\n\nDid you perhaps intend to list these hacks in a footnote or link to a \nrepository containing them?  (I can't find the counterpart of that [1].)\n\nRené\n"}]}