{"thread":{"id":"38810","subject":"Bug in fetch-pack.c, please confirm","startedAt":"2015-03-15T06:37:37Z","lastAt":"2015-03-19T21:09:44Z","messageCount":14,"participants":["Kyle J. McKay","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"257706","messageId":"0758b2029b41448a77a4e4df1c4e406@74d39fa044aa309eaea14b9f57fe79c","threadId":"38810","inReplyTo":null,"subject":"Bug in fetch-pack.c, please confirm","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-15T06:37:37Z","receivedAt":"2015-03-15T06:37:37Z","isPatch":false,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Hi guys,\n\nSo I was looking at fetch-pack.c (from master @ 52cae643, but I think \nit's the same everywhere):\n\n# Lines 626-648\n\n for (retval = 1, ref = *refs; ref ; ref = ref->next) {\n const unsigned char *remote = ref->old_sha1;\n unsigned char local[20];\n struct object *o;\n\n o = lookup_object(remote);\n if (!o || !(o->flags & COMPLETE)) {\n retval = 0;\n if (!args->verbose)\n continue;\n fprintf(stderr,\n \"want %s (%s)\\n\", sha1_to_hex(remote),\n ref->name);\n continue;\n }\n\n hashcpy(ref->new_sha1, local);\n if (!args->verbose)\n continue;\n fprintf(stderr,\n \"already have %s (%s)\\n\", sha1_to_hex(remote),\n ref->name);\n }\n\nPeff, weren't you having some issue with want and have and hide refs?\n\nTell me please how the \"local\" variable above gets initialized?\n\nIt's declared on the stack inside the for loop scope so only \nguaranteed to have garbage.\n\nIt's passed to hashcpy which has this prototype:\n\n inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src);\n\nSo it looks to me like garbage is copied into rev->new_sha1, yes?\n\nSomething's very wrong here.\n\nIt looks to me like the bug was introduced in 49bb805e (Do not ask for \nobjects known to be complete. 2005-10-19).\n\nI've taken a stab a a fix below.\n\n-Kyle\n\n-- 8< --\nSubject: [PATCH] fetch-pack.c: do not use uninitialized sha1 value\n\nSince 49bb805e (Do not ask for objects known to be complete. 2005-10-19)\nwhen the read_ref call was replaced with a parse_object call, the\nautomatic variable 'local' containing an sha1 has been left uninitialized.\n\nSubsequently in 1baaae5e (Make maximal use of the remote refs, 2005-10-28)\nthe parse_object call was replaced with a lookup_object call but still\nthe 'local' variable was left uninitialized.\n\nHowever, it's used as the source to update another sha1 value in the case\nthat we already have it and in that case the other ref will end up with\nwhatever garbage was in the 'local' variable.\n\nFix this by removing the 'local' variable and using the value from the\nresult of the lookup_object call instead.\n\nSigned-off-by: Kyle J. McKay \n---\n fetch-pack.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 655ee642..c0b4d84f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -621,29 +621,28 @@ static int everything_local(struct fetch_pack_args *args,\n \t\t}\n \t}\n \n \tfilter_refs(args, refs, sought, nr_sought);\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n-\t\tunsigned char local[20];\n \t\tstruct object *o;\n \n \t\to = lookup_object(remote);\n \t\tif (!o || !(o->flags & COMPLETE)) {\n \t\t\tretval = 0;\n \t\t\tif (!args->verbose)\n \t\t\t\tcontinue;\n \t\t\tfprintf(stderr,\n \t\t\t\t\"want %s (%s)\\n\", sha1_to_hex(remote),\n \t\t\t\tref->name);\n \t\t\tcontinue;\n \t\t}\n \n-\t\thashcpy(ref->new_sha1, local);\n+\t\thashcpy(ref->new_sha1, o->sha1);\n \t\tif (!args->verbose)\n \t\t\tcontinue;\n \t\tfprintf(stderr,\n \t\t\t\"already have %s (%s)\\n\", sha1_to_hex(remote),\n \t\t\tref->name);\n \t}\n \treturn retval;\n---\n"},{"id":"257710","messageId":"xmqqa8zevhya.fsf@gitster.dls.corp.google.com","threadId":"38810","inReplyTo":"0758b2029b41448a77a4e4df1c4e406@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T07:27:25Z","receivedAt":"2015-03-15T07:27:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> Hi guys,\n>\n> So I was looking at fetch-pack.c (from master @ 52cae643, but I think \n> it's the same everywhere):\n>\n> # Lines 626-648\n>\n\n49bb805e (Do not ask for objects known to be complete., 2005-10-19)\nseems to lose the assignment to local[].\n\n> Something's very wrong here.\n>\n> It looks to me like the bug was introduced in 49bb805e (Do not ask for \n> objects known to be complete. 2005-10-19).\n\nI read that \"lines 626-648\", stopped reading your message and did a\nblame session myself, and arrived at the same culprit.\n\nThe very original code that read from the ref directly here; I do\nnot think it was an attempt to catch a race condition where\nref->old_sha1 got stale and the ref on the filesystem has a newer\nvalue. Hence, I think copying from o->sha1 like you did is a fix\nthat is the most faithful to the original code.\n\no is lookup_object(remote) which is lookup_object(ref->old_sha1);\nthat makes o->sha1 always be ref->old_sha1, so either would be fine,\nbut o->sha1 would be more appropriate in this codeflow. We grab o,\nif it is NULL or if we do not like it, we do something and continue,\notherwise we like that o so grabbing o->sha1 out of it makes the\nlogic look flowing right, at least to me ;-)\n\n> -- 8< --\n> Subject: [PATCH] fetch-pack.c: do not use uninitialized sha1 value\n>\n> Since 49bb805e (Do not ask for objects known to be complete. 2005-10-19)\n> when the read_ref call was replaced with a parse_object call, the\n> automatic variable 'local' containing an sha1 has been left uninitialized.\n>\n> Subsequently in 1baaae5e (Make maximal use of the remote refs, 2005-10-28)\n> the parse_object call was replaced with a lookup_object call but still\n> the 'local' variable was left uninitialized.\n>\n> However, it's used as the source to update another sha1 value in the case\n> that we already have it and in that case the other ref will end up with\n> whatever garbage was in the 'local' variable.\n>\n> Fix this by removing the 'local' variable and using the value from the\n> result of the lookup_object call instead.\n>\n> Signed-off-by: Kyle J. McKay \n> ---\n> fetch-pack.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 655ee642..c0b4d84f 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -621,29 +621,28 @@ static int everything_local(struct fetch_pack_args *args,\n> \t\t}\n> \t}\n> \n> \tfilter_refs(args, refs, sought, nr_sought);\n> \n> \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n> \t\tconst unsigned char *remote = ref->old_sha1;\n> -\t\tunsigned char local[20];\n> \t\tstruct object *o;\n> \n> \t\to = lookup_object(remote);\n> \t\tif (!o || !(o->flags & COMPLETE)) {\n> \t\t\tretval = 0;\n> \t\t\tif (!args->verbose)\n> \t\t\t\tcontinue;\n> \t\t\tfprintf(stderr,\n> \t\t\t\t\"want %s (%s)\\n\", sha1_to_hex(remote),\n> \t\t\t\tref->name);\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\thashcpy(ref->new_sha1, local);\n> +\t\thashcpy(ref->new_sha1, o->sha1);\n> \t\tif (!args->verbose)\n> \t\t\tcontinue;\n> \t\tfprintf(stderr,\n> \t\t\t\"already have %s (%s)\\n\", sha1_to_hex(remote),\n> \t\t\tref->name);\n> \t}\n> \treturn retval;\n> ---\n"},{"id":"257711","messageId":"xmqq3856vhtt.fsf@gitster.dls.corp.google.com","threadId":"38810","inReplyTo":"xmqqa8zevhya.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T07:30:06Z","receivedAt":"2015-03-15T07:30:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Kyle J. McKay\" writes:\n>\n>> Hi guys,\n>>\n>> So I was looking at fetch-pack.c (from master @ 52cae643, but I think \n>> it's the same everywhere):\n>>\n> ...\n>> -\t\thashcpy(ref->new_sha1, local);\n>> +\t\thashcpy(ref->new_sha1, o->sha1);\n>> \t\tif (!args->verbose)\n>> \t\t\tcontinue;\n>> \t\tfprintf(stderr,\n>> \t\t\t\"already have %s (%s)\\n\", sha1_to_hex(remote),\n>> \t\t\tref->name);\n>> \t}\n>> \treturn retval;\n>> ---\n\nOne thing I wonder is if this hashcpy() is doing anything useful,\nthough. Is ref->new_sha1 used after we are done in this codepath,\nor is the reason nobody noticed it is because it does not matter\nwhatever garbage is in that field nobody looks at it?\n"},{"id":"257756","messageId":"471A97CA-0004-48F7-B6C9-50266EE370D4@gmail.com","threadId":"38810","inReplyTo":"xmqq3856vhtt.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-03-15T22:21:17Z","receivedAt":"2015-03-15T22:21:17Z","isPatch":false,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Mar 15, 2015, at 00:30, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n>> \"Kyle J. McKay\" writes:\n>>\n>>> Hi guys,\n>>>\n>>> So I was looking at fetch-pack.c (from master @ 52cae643, but I \n>>> think\n>>> it's the same everywhere):\n>>>\n>> ...\n>>> -\t\thashcpy(ref->new_sha1, local);\n>>> +\t\thashcpy(ref->new_sha1, o->sha1);\n>>> \t\tif (!args->verbose)\n>>> \t\t\tcontinue;\n>>> \t\tfprintf(stderr,\n>>> \t\t\t\"already have %s (%s)\\n\", sha1_to_hex(remote),\n>>> \t\t\tref->name);\n>>> \t}\n>>> \treturn retval;\n>>> ---\n>\n> One thing I wonder is if this hashcpy() is doing anything useful,\n> though. Is ref->new_sha1 used after we are done in this codepath,\n> or is the reason nobody noticed it is because it does not matter\n> whatever garbage is in that field nobody looks at it?\n\nMy thoughts exactly. hence the \"please confirm\" request. I'm not \nfamiliar enough with the fetch pack code to know the answer off the \ntop of my head. I was hoping someone else who's been in the fetch \npack code recently (*Hi Peff*) might just already know. :)\n"},{"id":"257766","messageId":"20150316011343.GA928@peff.net","threadId":"38810","inReplyTo":"0758b2029b41448a77a4e4df1c4e406@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-16T01:13:43Z","receivedAt":"2015-03-16T01:13:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 14, 2015 at 11:37:37PM -0700, Kyle J. McKay wrote:\n\n> Peff, weren't you having some issue with want and have and hide refs?\n\nYes, but the problem was on the server side. I didn't look at this code\nat all. :)\n\n> Tell me please how the \"local\" variable above gets initialized?\n\nSo I think we all agree that this is bogus code, and the fix you\nproposed is reasonable (but spoiler alert, if you read to the end, I\nthink we can do something even more extreme). Like Junio, I am puzzled\nnot by the bug itself, but by its lack of effect over the past 10 years.\n\nI'm not all that familiar with this code, so I started with some rather\nblunt-hammer tracing.\n\nIf you replace that hashcpy with \"exit(1)\", you get quite a few failures\nin the test suite. So we really are running the code. Interestingly, if\nyou replace it with \"hashclr(ref->old_sha1)\" (note old, not new, so\nimpacting the sha1 that we know is actually valid), you still get some\nfailures, but many fewer.\n\nIt looks like one of the ways to hit this code path is by doing a clone\nin which we don't actually need any objects (e.g., using \"--reference\").\nClone runs fetch-pack to acquire the objects, but then throws away the\nreturned refs and writes its own.\n\nBut there are still some failures, so clearly some code paths actually\ndo look at the resulting ref. But nobody seems to care about\nref->new_sha1. Let's take one of the cases that the hashclr() experiment\nfound and trace that. In t1507, we do a \"git fetch\" that hits this case.\nI instrumented it like this:\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 1978947..e2a192d 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -82,7 +82,7 @@ test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{u\n test_expect_success 'my-side@{u} resolves to correct commit' '\n \tgit checkout side &&\n \ttest_commit 5 &&\n-\t(cd clone && git fetch) &&\n+\t(cd clone && gdb --args ../../../git fetch /dev/tty) &&\n \ttest 2 = \"$(commit_subject my-side)\" &&\n \ttest 5 = \"$(commit_subject my-side@{u})\"\n '\n\nIf we break in everything_local at that hashcpy, we can see:\n\n (gdb) print ref->name\n $1 = 0x8422b0 \"refs/heads/master\"\n (gdb) print sha1_to_hex(ref->old_sha1)\n $2 = 0x815a40 \"8f489d01d0cc65c3b0f09504ec50b5ed02a70bd5\"\n (gdb) print sha1_to_hex(ref->new_sha1)\n $3 = 0x815a69 \"f2795a000000000056134b\", '0' , \"8000000\"\n\nSo that's the bogus value copied in. Presumably it's random bytes and\nnot bleed over from some other sha1, as it's rather unlikely to have\nthat many zeroes in a real hash.\n\nIf we then leave everything_local, we see that the bogus value comes out\nto do_fetch_pack via the \"ref\" parameter. We feed that into find_common,\nbut it only looks at ref->old_sha1, the other side. We keep going, and\ndo_fetch_pack returns the bogus ref up to fetch_pack(). That in turn\npasses it up to fetch_refs_via_pack.\n \nAnd there we stop. We don't pass the \"refs\" list out of that function\n(which, as an aside, is probably a leak). Instead, we depend on the list\nof heads we already knew in the \"to_fetch\" array. That comes from\nprocessing the earlier list of refs returned from get_refs_via_connect.\n\nI think it was the case once upon a time that fetch-pack did more of the\n\"which refs to fetch, and how to update them\" logic. The pipeline for\nfetch is now more like:\n\n 1. get_refs_via_connect returns a list of refs\n\n 2. caller munges the list of refs, based on refspecs\n\n 3. fetch_refs_via_pack takes that list of refs as input\n\nOf course, this being git, there's always a way to try to access the\n\"old\" code paths. :)\n\nYou can do something like:\n\n git init\n git commit --allow-empty -m foo\n git fetch-pack --all .\n\nwhich hits the same code path, but actually retains the return value\nfrom fetch_pack(). But even there, it looks like we don't look at\nref->new_sha1 at all. fetch-pack doesn't update refs at all, but just\nprints the list of new values to stdout.\n\nSo I think there is literally no code path that ever looks at the bogus\nsha1. We could just drop that hashcpy() entirely.\n\nThat doesn't mean there isn't an additional bug lurking. That is,\n_should_ somebody be caring about that value? I don't think so. The\nold/new pairs in a \"struct ref\" are meaningful as \"I proposed to update\nto X, and we are at Y\". But this list of refs does not have anything to\ndo with the update of local refs. It is only \"what is the value of the\nref on the other side\". The local refs are taken care of in a separate\nlist.\n\n-Peff\n"},{"id":"258035","messageId":"xmqqfv90khpd.fsf@gitster.dls.corp.google.com","threadId":"38810","inReplyTo":"20150316011343.GA928@peff.net","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T17:41:50Z","receivedAt":"2015-03-19T17:41:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> And there we stop. We don't pass the \"refs\" list out of that function\n> (which, as an aside, is probably a leak). Instead, we depend on the list\n> of heads we already knew in the \"to_fetch\" array. That comes from\n> processing the earlier list of refs returned from get_refs_via_connect.\n> ...\n> That doesn't mean there isn't an additional bug lurking. That is,\n> _should_ somebody be caring about that value? I don't think so. The\n> old/new pairs in a \"struct ref\" are meaningful as \"I proposed to update\n> to X, and we are at Y\". But this list of refs does not have anything to\n> do with the update of local refs. It is only \"what is the value of the\n> ref on the other side\". The local refs are taken care of in a separate\n> list.\n\nCorrect. When we want to insert some function to allow users/hooks\nto tweak the result of update, we might want to make sure that we\nare passing the final list of refs to that function, but the purpose\nof this function is not to make such a modification.\n\nJust to make sure we do not keep this hanging forever and eventually\nforget it, I'm planning to queue this.\n\nThanks.\n\n-- >8 --\nFrom: Jeff King \nDate: Sun, 15 Mar 2015 21:13:43 -0400\nSubject: [PATCH] fetch-pack: remove dead assignment to ref->new_sha1\n\nIn everything_local(), we used to assign the current ref's value\nfound in ref->old_sha1 to ref->new_sha1 when we already have all the\nnecessary objects to complete the history leading to that commit.\nThis copying was broken at 49bb805e (Do not ask for objects known to\nbe complete., 2005-10-19) and ever since we instead stuffed a random\nbytes in ref->new_sha1 here. No code complained or failed due to this\nbreakage.\n\nIt turns out that no codepath that comes after this assignment even\nlooks at ref->new_sha1 at all.\n\n - The only caller of everything_local(), do_fetch_pack(), returns\n this list of ref, whose element has bogus new_sha1 values, to its\n caller. It does not look at the elements and acts on them.\n\n - The only caller of do_fetch_pack(), fetch_pack(), returns this\n list to its caller. It does not look at the elements and acts on\n them.\n\n - One of the two callers of fetch_pack() is cmd_fetch_pack(), the\n top-level that implements \"git fetch-pack\". The only thing it\n looks at in the elements of the returned ref list is the old_sha1\n and name fields.\n\n - The other caller of fetch_pack() is fetch_refs_via_pack() in the\n transport layer, which is a helper that implements \"git fetch\".\n It only cares about whether the returned list is empty (i.e.\n failed to fetch anything).\n\nJust drop the bogus assignment, that is not even necessary. The\nremote-tracking refs are updated based on a different list and not\nusing the ref list being manipulated by this codepath; the caller\ndo_fetch_pack() created a copy of that real ref list and passed the\ncopy down to this function, and modifying the elements here does not\naffect anything.\n\nNoticed-by: Kyle J. McKay \nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n fetch-pack.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 655ee64..6f0c0e1 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -625,7 +625,6 @@ static int everything_local(struct fetch_pack_args *args,\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n-\t\tunsigned char local[20];\n \t\tstruct object *o;\n \n \t\to = lookup_object(remote);\n@@ -638,8 +637,6 @@ static int everything_local(struct fetch_pack_args *args,\n \t\t\t\tref->name);\n \t\t\tcontinue;\n \t\t}\n-\n-\t\thashcpy(ref->new_sha1, local);\n \t\tif (!args->verbose)\n \t\t\tcontinue;\n \t\tfprintf(stderr,\n-- \n2.3.3-377-gdf43604\n"},{"id":"258040","messageId":"20150319185517.GB8788@peff.net","threadId":"38810","inReplyTo":"xmqqfv90khpd.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T18:55:17Z","receivedAt":"2015-03-19T18:55:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 10:41:50AM -0700, Junio C Hamano wrote:\n\n> Just to make sure we do not keep this hanging forever and eventually\n> forget it, I'm planning to queue this.\n\nThanks for following up. A few minor nits (and maybe a more serious\nproblem) on the explanation in the commit message:\n\n> be complete., 2005-10-19) and ever since we instead stuffed a random\n> bytes in ref->new_sha1 here.\n\ns/a random/random/\n\n> It turns out that no codepath that comes after this assignment even\n> looks at ref->new_sha1 at all.\n> \n> - The only caller of everything_local(), do_fetch_pack(), returns\n> this list of ref, whose element has bogus new_sha1 values, to its\n> caller. It does not look at the elements and acts on them.\n\ns/list of ref/list of refs/ ? Or did you mean \"the list we store in the\n'ref' variable\"?\n\nI'm not sure what the final sentence means. Should it be \"It does not\nlook at the elements nor act on them\"?\n\ndo_fetch_pack actually does pass them down to find_common. But the\nlatter does not look at ref->new_sha1, so we're OK.\n\n> - The only caller of do_fetch_pack(), fetch_pack(), returns this\n> list to its caller. It does not look at the elements and acts on\n> them.\n\nDitto on the wording in the final sentence here (but it is correct in\nthis case that we do not touch the list at all).\n\n> - The other caller of fetch_pack() is fetch_refs_via_pack() in the\n> transport layer, which is a helper that implements \"git fetch\".\n> It only cares about whether the returned list is empty (i.e.\n> failed to fetch anything).\n\nSo I thought I would follow this up by adding a free_refs() call in\nfetch_refs_via_pack. After all, we just leak that list, right?\n\nIf only it were so simple. It turns out that the list returned from\nfetch_pack() is _mostly_ a copy of the incoming refs list we give it.\nBut in filter_refs(), if allow_tip_sha1_in_want is set, we actually add\nthe unmatched \"sought\" refs here, too.\n\nWhich means we may be setting new_sha1 in one of these \"sought\" refs,\nand that broadens the scope of code that might be affected by the patch\nwe have been discussing. However, I think the filter_refs code is wrong,\nand should be making a copy of the sought refs to put in the new list.\n\nI'm working up a few patches in that area, which I'll send out in a few\nminutes. Once that is done, then I think the explanation you give above\nwould be correct.\n\n-Peff\n"},{"id":"258041","messageId":"xmqqtwxgizg9.fsf@gitster.dls.corp.google.com","threadId":"38810","inReplyTo":"20150319185517.GB8788@peff.net","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T19:01:26Z","receivedAt":"2015-03-19T19:01:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> - The only caller of everything_local(), do_fetch_pack(), returns\n>> this list of ref, whose element has bogus new_sha1 values, to its\n>> caller. It does not look at the elements and acts on them.\n>\n> I'm not sure what the final sentence means. Should it be \"It does not\n> look at the elements nor act on them\"?\n\nYes. \"It does not look at the new_sha1. It does not use the\ninformation to change its behaviour. Both of the previous two\nstatements are true.\" is what I meant.\n\n> do_fetch_pack actually does pass them down to find_common. But the\n> latter does not look at ref->new_sha1, so we're OK.\n\n>> - The other caller of fetch_pack() is fetch_refs_via_pack() in the\n>> transport layer, which is a helper that implements \"git fetch\".\n>> It only cares about whether the returned list is empty (i.e.\n>> failed to fetch anything).\n>\n> So I thought I would follow this up by adding a free_refs() call in\n> fetch_refs_via_pack. After all, we just leak that list, right?\n\nYeah, I agree.\n\n> I'm working up a few patches in that area, which I'll send out in a few\n> minutes. Once that is done, then I think the explanation you give above\n> would be correct.\n\nIf a follow-up is coming then I'd just drop this one. Thanks.\n"},{"id":"258047","messageId":"20150319203126.GA31663@peff.net","threadId":"38810","inReplyTo":"xmqqtwxgizg9.fsf@gitster.dls.corp.google.com","subject":"Re: Bug in fetch-pack.c, please confirm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T20:31:26Z","receivedAt":"2015-03-19T20:31:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 12:01:26PM -0700, Junio C Hamano wrote:\n\n> > I'm working up a few patches in that area, which I'll send out in a few\n> > minutes. Once that is done, then I think the explanation you give above\n> > would be correct.\n> \n> If a follow-up is coming then I'd just drop this one. Thanks.\n\nOK, here it is. Took me a bit longer than I expected, as I wanted to\nfigure out whether the second patch was actually fixing a bug (and if\nso, to add test coverage). Turns out that it is a real bug.\n\nThe final patch is what you sent, rebased on top (though there are not\nany code changes; the underlying commits make the _explanation_ true,\nbut no code change was required). I fixed up the nits I mentioned in my\nearlier email.\n\n [1/4]: filter_ref: avoid overwriting ref->old_sha1 with garbage\n [2/4]: filter_ref: make a copy of extra \"sought\" entries\n [3/4]: fetch_refs_via_pack: free extra copy of refs\n [4/4]: fetch-pack: remove dead assignment to ref->new_sha1\n\n-Peff\n"},{"id":"258049","messageId":"20150319203451.GA7666@peff.net","threadId":"38810","inReplyTo":"20150319203126.GA31663@peff.net","subject":"[PATCH 1/4] filter_ref: avoid overwriting ref->old_sha1 with garbage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T20:34:51Z","receivedAt":"2015-03-19T20:34:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the server supports allow_tip_sha1_in_want, then\nfetch-pack's filter_refs function tries to check whether a\nref is a request for a straight sha1 by running:\n\n if (get_sha1_hex(ref->name, ref->old_sha1))\n\t ...\n\nI.e., we are using get_sha1_hex to ask \"is this ref name a\nsha1?\". If it is true, then the contents of ref->old_sha1\nwill end up unchanged. But if it is false, then get_sha1_hex\nmakes no guarantees about what it has written. With a ref\nname like \"abcdefoo\", we would overwrite 3 bytes of\nref->old_sha1 before realizing that it was not a sha1.\n\nThis is likely not a problem in practice, as anything in\nrefs->name (besides a sha1) will start with \"refs/\", meaning\nthat we would notice on the first character that there is a\nproblem. Still, we are making assumptions about the state\nleft in the output when get_sha1_hex returns an error (e.g.,\nit could start from the end of the string, or error check\nthe values only once they were placed in the output). It's\nbetter to be defensive.\n\nWe could just check that we have exactly 40 characters of\nsha1. But let's be even more careful and make sure that we\nhave a 40-char hex refname that matches what is in old_sha1.\nThis is perhaps overly defensive, but spells out our\nassumptions clearly.\n\nSigned-off-by: Jeff King \n---\nThis one is not necessary for the others, of course, and we can drop it\nif you disagree with the reasoning.\n\nI wonder if the thinking in the original was that it was our\nresponsibility here to make sure that ref->old_sha1 was filled in. It is\nalways filled in by the caller who gives us \"sought\", which makes sense\nto me (this matches the rest of the \"sought\" entries, which come from\nreading the remote's ref list, and of course must fill in old_sha1 from\nthat list).\n\n fetch-pack.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 655ee64..058c258 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -544,10 +544,14 @@ static void filter_refs(struct fetch_pack_args *args,\n \t/* Append unmatched requests to the list */\n \tif (allow_tip_sha1_in_want) {\n \t\tfor (i = 0; i < nr_sought; i++) {\n+\t\t\tunsigned char sha1[20];\n+\n \t\t\tref = sought[i];\n \t\t\tif (ref->matched)\n \t\t\t\tcontinue;\n-\t\t\tif (get_sha1_hex(ref->name, ref->old_sha1))\n+\t\t\tif (get_sha1_hex(ref->name, sha1) ||\n+\t\t\t ref->name[40] != '\\0' ||\n+\t\t\t hashcmp(sha1, ref->old_sha1))\n \t\t\t\tcontinue;\n \n \t\t\tref->matched = 1;\n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258050","messageId":"20150319203708.GB7666@peff.net","threadId":"38810","inReplyTo":"20150319203126.GA31663@peff.net","subject":"[PATCH 2/4] filter_ref: make a copy of extra \"sought\" entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T20:37:09Z","receivedAt":"2015-03-19T20:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the server supports allow_tip_sha1_in_want, we add any\nunmatched raw-sha1 entries in our \"sought\" list of refs to\nthe list of refs we will ask the other side for. We do so by\ninserting the original \"struct ref\" directly into our list,\nrather than making a copy. This has several problems.\n\nThe most minor problem is that one cannot ever free the\nresulting list; it contains structs that are copies of the\nremote refs (made earlier by fetch_pack) along with sought\nrefs that are referenced elsewhere.\n\nBut more importantly that we set the ref->next pointer to\nNULL, chopping off the remainder of any existing list that\nthe ref was a part of. We get the set of \"sought\" refs in\nan array rather than a linked list, but that array is often\nin turn generated from a list. The test modification in\nt5516 demonstrates this. Rather than fetching just an exact\nsha1, we fetch that sha1 plus another ref:\n\n - we build a linked list of refs to fetch when do_fetch\n calls get_ref_map; the exact sha1 is first, followed by\n the named ref (\"refs/heads/extra\" in this case).\n\n - we pass that linked list to transport_fetch_ref, which\n squashes it into an array of pointers\n\n - that array goes to fetch_pack, which calls filter_ref.\n There we generate the want list from a mix of what the\n remote side has advertised, and the \"sought\" entry for\n the exact sha1. We set the sought entry's \"next\" pointer\n to NULL.\n\n - after we return from transport_fetch_refs, we then try\n to update the refs by following the linked list. But our\n list is now truncated, and we do not update\n refs/heads/extra at all.\n\nWe can fix this by making a copy of the ref. There's nothing\nthat fetch_pack does to it that must be reflected in the\noriginal \"sought\" list (and indeed, if that were the case we\nwould have a serious bug, because it is only exact-sha1\nentries which are treated this way).\n\nSigned-off-by: Jeff King \n---\nI always feel a little dirty modifying an existing test rather than\nwriting a new one that more clearly demonstrates what we are testing.\nBut the setup involved in the exact-sha1 fetch is a little complicated,\nso this seemed easiest (and anybody blaming the change can hopefully\nrely on the explanation above to understand what is going on).\n\n fetch-pack.c | 5 ++---\n t/t5516-fetch-push.sh | 13 ++++++++++---\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 058c258..84d5a66 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -555,9 +555,8 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\t\t\tcontinue;\n \n \t\t\tref->matched = 1;\n-\t\t\t*newtail = ref;\n-\t\t\tref->next = NULL;\n-\t\t\tnewtail = &ref->next;\n+\t\t\t*newtail = copy_ref(ref);\n+\t\t\tnewtail = &(*newtail)->next;\n \t\t}\n \t}\n \t*refs = newlist;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 630885d..5e04d64 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1107,9 +1107,16 @@ test_expect_success 'fetch exact SHA1' '\n \t\t\tgit config uploadpack.allowtipsha1inwant true\n \t\t) &&\n \n-\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy &&\n-\t\tresult=$(git rev-parse --verify refs/heads/copy) &&\n-\t\ttest \"$the_commit\" = \"$result\"\n+\t\tgit fetch -v ../testrepo $the_commit:refs/heads/copy master:refs/heads/extra &&\n+\t\tcat >expect <<-EOF &&\n+\t\t$the_commit\n+\t\t$the_first_commit\n+\t\tEOF\n+\t\t{\n+\t\t\tgit rev-parse --verify refs/heads/copy &&\n+\t\t\tgit rev-parse --verify refs/heads/extra\n+\t\t} >actual &&\n+\t\ttest_cmp expect actual\n \t)\n '\n \n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258051","messageId":"20150319203834.GC7666@peff.net","threadId":"38810","inReplyTo":"20150319203126.GA31663@peff.net","subject":"[PATCH 3/4] fetch_refs_via_pack: free extra copy of refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T20:38:35Z","receivedAt":"2015-03-19T20:38:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When fetch_refs_via_pack calls fetch_pack(), we pass a\nlist of refs to fetch, and the function returns either a\ncopy of that list, with the fetched items filled in, or\nNULL. We check the return value to see whether the fetch was\nsuccessful, but do not otherwise look at the copy, and\nsimply leak it at the end of the function.\n\nSigned-off-by: Jeff King \n---\nWithout patch 2, this segfaults in t5516. :)\n\n transport.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 0694a7c..b1953b2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -519,7 +519,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct git_transport_data *data = transport->data;\n-\tconst struct ref *refs;\n+\tstruct ref *refs;\n \tchar *dest = xstrdup(transport->url);\n \tstruct fetch_pack_args args;\n \tstruct ref *refs_tmp = NULL;\n@@ -552,15 +552,17 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\t\t &transport->pack_lockfile);\n \tclose(data->fd[0]);\n \tclose(data->fd[1]);\n-\tif (finish_connect(data->conn))\n+\tif (finish_connect(data->conn)) {\n+\t\tfree_refs(refs);\n \t\trefs = NULL;\n+\t}\n \tdata->conn = NULL;\n \tdata->got_remote_heads = 0;\n \tdata->options.self_contained_and_connected =\n \t\targs.self_contained_and_connected;\n \n \tfree_refs(refs_tmp);\n-\n+\tfree_refs(refs);\n \tfree(dest);\n \treturn (refs ? 0 : -1);\n }\n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258052","messageId":"20150319203935.GD7666@peff.net","threadId":"38810","inReplyTo":"20150319203126.GA31663@peff.net","subject":"[PATCH 4/4] fetch-pack: remove dead assignment to ref->new_sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T20:39:36Z","receivedAt":"2015-03-19T20:39:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In everything_local(), we used to assign the current ref's value\nfound in ref->old_sha1 to ref->new_sha1 when we already have all the\nnecessary objects to complete the history leading to that\ncommit. This copying was broken at 49bb805e (Do not ask for\nobjects known to be complete., 2005-10-19) and ever since we\ninstead stuffed a random bytes in ref->new_sha1 here. No\ncode complained or failed due to this breakage.\n\nIt turns out that no code path that comes after this\nassignment even looks at ref->new_sha1 at all.\n\n - The only caller of everything_local(), do_fetch_pack(),\n returns this list of refs, whose element has bogus\n new_sha1 values, to its caller. It does not look at the\n elements itself, but does pass them to find_common, which\n looks only at the name and old_sha1 fields.\n\n - The only caller of do_fetch_pack(), fetch_pack(), returns this\n list to its caller. It does not look at the elements nor act on\n them.\n\n - One of the two callers of fetch_pack() is cmd_fetch_pack(), the\n top-level that implements \"git fetch-pack\". The only thing it\n looks at in the elements of the returned ref list is the old_sha1\n and name fields.\n\n - The other caller of fetch_pack() is fetch_refs_via_pack() in the\n transport layer, which is a helper that implements \"git fetch\".\n It only cares about whether the returned list is empty (i.e.\n failed to fetch anything).\n\nJust drop the bogus assignment, that is not even necessary. The\nremote-tracking refs are updated based on a different list and not\nusing the ref list being manipulated by this code path; the caller\ndo_fetch_pack() created a copy of that real ref list and passed the\ncopy down to this function, and modifying the elements here does not\naffect anything.\n\nNoticed-by: Kyle J. McKay \nSigned-off-by: Jeff King \n---\n fetch-pack.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 84d5a66..48526aa 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -628,7 +628,6 @@ static int everything_local(struct fetch_pack_args *args,\n \n \tfor (retval = 1, ref = *refs; ref ; ref = ref->next) {\n \t\tconst unsigned char *remote = ref->old_sha1;\n-\t\tunsigned char local[20];\n \t\tstruct object *o;\n \n \t\to = lookup_object(remote);\n@@ -641,8 +640,6 @@ static int everything_local(struct fetch_pack_args *args,\n \t\t\t\tref->name);\n \t\t\tcontinue;\n \t\t}\n-\n-\t\thashcpy(ref->new_sha1, local);\n \t\tif (!args->verbose)\n \t\t\tcontinue;\n \t\tfprintf(stderr,\n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258056","messageId":"xmqq7fucitif.fsf@gitster.dls.corp.google.com","threadId":"38810","inReplyTo":"20150319203451.GA7666@peff.net","subject":"Re: [PATCH 1/4] filter_ref: avoid overwriting ref->old_sha1 with garbage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T21:09:44Z","receivedAt":"2015-03-19T21:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder if the thinking in the original was that it was our\n> responsibility here to make sure that ref->old_sha1 was filled in.\n\nI am reasonably sure that is the (perhaps mistaken) reasoning behind\nthe use of old_sha1 as the second parameter to get_sha1_hex().\n\n> It is\n> always filled in by the caller who gives us \"sought\", which makes sense\n> to me (this matches the rest of the \"sought\" entries, which come from\n> reading the remote's ref list, and of course must fill in old_sha1 from\n> that list).\n\nI see that sought is populated by reading the command line of \"git\nfetch-pack\", and for a 40-hex request ref->old_sha1 is already\nfilled there, so I agree that it is redundant to try filling it in\nfilter_refs().\n\nThanks.\n\n> fetch-pack.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 655ee64..058c258 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -544,10 +544,14 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t/* Append unmatched requests to the list */\n> \tif (allow_tip_sha1_in_want) {\n> \t\tfor (i = 0; i < nr_sought; i++) {\n> +\t\t\tunsigned char sha1[20];\n> +\n> \t\t\tref = sought[i];\n> \t\t\tif (ref->matched)\n> \t\t\t\tcontinue;\n> -\t\t\tif (get_sha1_hex(ref->name, ref->old_sha1))\n> +\t\t\tif (get_sha1_hex(ref->name, sha1) ||\n> +\t\t\t ref->name[40] != '\\0' ||\n> +\t\t\t hashcmp(sha1, ref->old_sha1))\n> \t\t\t\tcontinue;\n> \n> \t\t\tref->matched = 1;\n"}]}