{"thread":{"id":"10207","subject":"[PATCH] Fixed crash in fetching remote tags when there is not tags.","startedAt":"2007-10-09T08:51:07Z","lastAt":"2007-10-14T21:26:05Z","messageCount":10,"participants":["Väinö Järvelä","Alex Riesen","Jeff King","Shawn O. Pearce","Lars Hjemli"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"55244","messageId":"1191919868-4963-1-git-send-email-v@pp.inet.fi","threadId":"10207","inReplyTo":null,"subject":"[PATCH] Added a test for fetching remote tags when there is not tags.","fromName":"Väinö Järvelä","fromEmail":"v@pp.inet.fi","sentAt":"2007-10-09T08:51:07Z","receivedAt":"2007-10-09T08:51:07Z","isPatch":true,"sender":{"key":"v@pp.inet.fi","avatar":null},"body":"When a user runs \"git fetch -t\", git crashes when it doesn't find any\ntags on the remote repository.\n\nSigned-off-by: Väinö Järvelä \n---\n t/t5510-fetch.sh | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 73a4e3c..40ebf2e 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -67,6 +67,18 @@ test_expect_success \"fetch test for-merge\" '\n \tcut -f -2 .git/FETCH_HEAD >actual &&\n \tdiff expected actual'\n \n+test_expect_success 'fetch tags when there is no tags' '\n+\n+ cd \"$D\" &&\n+\n+ mkdir notags &&\n+ cd notags &&\n+ git init &&\n+\n+ git fetch -t ..\n+\n+'\n+\n test_expect_success 'fetch following tags' '\n \n \tcd \"$D\" &&\n-- \n1.5.3.4.1156.g5407-dirty\n"},{"id":"55243","messageId":"1191919868-4963-2-git-send-email-v@pp.inet.fi","threadId":"10207","inReplyTo":"1191919868-4963-1-git-send-email-v@pp.inet.fi","subject":"[PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Väinö Järvelä","fromEmail":"v@pp.inet.fi","sentAt":"2007-10-09T08:51:08Z","receivedAt":"2007-10-09T08:51:08Z","isPatch":true,"sender":{"key":"v@pp.inet.fi","avatar":null},"body":"Signed-off-by: Väinö Järvelä \n---\n remote.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex e7d735b..5e92378 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -537,6 +537,8 @@ static int count_refspec_match(const char *pattern,\n \n static void tail_link_ref(struct ref *ref, struct ref ***tail)\n {\n+\tif (!ref) return;\n+\t\n \t**tail = ref;\n \twhile (ref->next)\n \t\tref = ref->next;\n-- \n1.5.3.4.1156.g5407-dirty\n"},{"id":"55253","messageId":"81553116-3A4F-4526-A772-9A43C53D3E22@pp.inet.fi","threadId":"10207","inReplyTo":"1191919868-4963-2-git-send-email-v@pp.inet.fi","subject":"Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Väinö Järvelä","fromEmail":"v@pp.inet.fi","sentAt":"2007-10-09T10:52:01Z","receivedAt":"2007-10-09T10:52:01Z","isPatch":true,"sender":{"key":"v@pp.inet.fi","avatar":null},"body":"Hi,\n\nThese patches should have been numbered, sorry, The test patch was \nsupposed be number 1 of 2.\n\nThe patches were done on top of next. The fix should be checked, as \nit's mostly a quick fix to get it to work, probably not the correct \nway to fix that bug, or is it?\n\n--\nVäinö\n\nOn Oct 9, 2007, at 11:51, Väinö Järvelä wrote:\n\n> Signed-off-by: Väinö Järvelä \n> ---\n> remote.c | 2 ++\n> 1 files changed, 2 insertions(+), 0 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index e7d735b..5e92378 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -537,6 +537,8 @@ static int count_refspec_match(const char \n> *pattern,\n>\n> static void tail_link_ref(struct ref *ref, struct ref ***tail)\n> {\n> +\tif (!ref) return;\n> +\t\n> \t**tail = ref;\n> \twhile (ref->next)\n> \t\tref = ref->next;\n> -- \n> 1.5.3.4.1156.g5407-dirty\n>\n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"55296","messageId":"20071009182043.GA2997@steel.home","threadId":"10207","inReplyTo":"81553116-3A4F-4526-A772-9A43C53D3E22@pp.inet.fi","subject":"Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-09T18:20:43Z","receivedAt":"2007-10-09T18:20:43Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Väinö Järvelä, Tue, Oct 09, 2007 12:52:01 +0200:\n> Hi,\n> \n> These patches should have been numbered, sorry, The test patch was \n> supposed be number 1 of 2.\n> \n> The patches were done on top of next. The fix should be checked, as \n> it's mostly a quick fix to get it to work, probably not the correct \n> way to fix that bug, or is it?\n\nYour test does not crash on my system and your fix is obviously bogus.\nJust take a look at the only call site of the tail_link_ref: ret\ncannot be NULL. alloc_ref will crash in memset, if this were the case.\n\nIf you can reproduce the crash reliably, try compiling git with -g and\nrun it in your test with valgrind or gdb (assuming these are available\nto you).\n"},{"id":"55331","messageId":"20071010051034.GA30834@coredump.intra.peff.net","threadId":"10207","inReplyTo":"20071009182043.GA2997@steel.home","subject":"Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-10-10T05:10:35Z","receivedAt":"2007-10-10T05:10:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 09, 2007 at 08:20:43PM +0200, Alex Riesen wrote:\n\n> Your test does not crash on my system and your fix is obviously bogus.\n> Just take a look at the only call site of the tail_link_ref: ret\n> cannot be NULL. alloc_ref will crash in memset, if this were the case.\n\nHis work is almost certainly on top of next, which crashes reliably with\nthe test and has an additional call site for tail_link_ref. Aside from\nsome trailing whitespace in the patch, I think his fix is reasonable.\n\n-Peff\n"},{"id":"55416","messageId":"20071010212735.GB16635@steel.home","threadId":"10207","inReplyTo":"20071010051034.GA30834@coredump.intra.peff.net","subject":"Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-10T21:27:35Z","receivedAt":"2007-10-10T21:27:35Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jeff King, Wed, Oct 10, 2007 07:10:35 +0200:\n> On Tue, Oct 09, 2007 at 08:20:43PM +0200, Alex Riesen wrote:\n> \n> > Your test does not crash on my system and your fix is obviously bogus.\n> > Just take a look at the only call site of the tail_link_ref: ret\n> > cannot be NULL. alloc_ref will crash in memset, if this were the case.\n> \n> His work is almost certainly on top of next, which crashes reliably with\n> the test and has an additional call site for tail_link_ref. Aside from\n> some trailing whitespace in the patch, I think his fix is reasonable.\n\nAch, I see.\n\nStill, I'd suggest move the test into the caller, firstly because it\nis the only place that special. Also, I can't think of a proper reason\nto add a NULL ref to a reflist, and so the crashing tail_link_ref will\nhelp us find the callers which use tail_link_ref incorrectly\n(illogically too).\n\nAs the result of patter expansion can be NULL (empty pattern, as it\nseems), lets just check for it. I parked the patch below locally.\n\ndiff --git a/remote.c b/remote.c\nindex 5e92378..58d63ed 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -884,7 +884,8 @@ int get_fetch_map(struct ref *remote_refs,\n \t\t\t rm->peer_ref->name);\n \t}\n \n-\ttail_link_ref(ref_map, tail);\n+\tif (ref_map)\n+\t\ttail_link_ref(ref_map, tail);\n \n \treturn 0;\n }\n"},{"id":"55418","messageId":"20071010213305.GB8518@sigill.intra.peff.net","threadId":"10207","inReplyTo":"20071010212735.GB16635@steel.home","subject":"Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-10-10T21:33:06Z","receivedAt":"2007-10-10T21:33:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 10, 2007 at 11:27:35PM +0200, Alex Riesen wrote:\n\n> Still, I'd suggest move the test into the caller, firstly because it\n> is the only place that special. Also, I can't think of a proper reason\n\nYes, I agree with that. I came very close to suggesting it in my other\nmail, but then realized I had never even looked at the surrounding code,\nand I ought not to be making assessments of how that data structure\nshould be used. But now there are two of us. :)\n\n-Peff\n"},{"id":"55522","messageId":"20071012040745.GC27899@spearce.org","threadId":"10207","inReplyTo":"20071010212735.GB16635@steel.home","subject":"Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-12T04:07:45Z","receivedAt":"2007-10-12T04:07:45Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Alex Riesen wrote:\n> Still, I'd suggest move the test into the caller, firstly because it\n> is the only place that special. Also, I can't think of a proper reason\n> to add a NULL ref to a reflist, and so the crashing tail_link_ref will\n> help us find the callers which use tail_link_ref incorrectly\n> (illogically too).\n> \n> As the result of patter expansion can be NULL (empty pattern, as it\n> seems), lets just check for it. I parked the patch below locally.\n> \n> diff --git a/remote.c b/remote.c\n> index 5e92378..58d63ed 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -884,7 +884,8 @@ int get_fetch_map(struct ref *remote_refs,\n> \t\t\t rm->peer_ref->name);\n> \t}\n> \n> -\ttail_link_ref(ref_map, tail);\n> +\tif (ref_map)\n> +\t\ttail_link_ref(ref_map, tail);\n> \n> \treturn 0;\n> }\n\nI disagree with Alex's argument above, because the result of the\npattern expansion can be NULL due to a pattern that matched nothing\nfrom the remote. This can easily happen if the user originally\nconfigured \"remote.$name.fetch = +refs/heads/subdir/*:...\" and\nthen the remote deletes all branches from refs/heads/subdir at some\npoint in the future.\n\nI think the above patch is the only thing to do here. Perhaps Alex\ncan write up a formal patch and send it to back to the list and CC\nLars Hjemli so he can put it into the patch queue.\n\n-- \nShawn.\n"},{"id":"55573","messageId":"20071012204004.GA6700@steel.home","threadId":"10207","inReplyTo":"20071012040745.GC27899@spearce.org","subject":"[PATCH] Fix a crash in ls-remote when refspec expands into nothing","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-10-12T20:40:04Z","receivedAt":"2007-10-12T20:40:04Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Originally-by: Väinö Järvelä \nSigned-off-by: Alex Riesen \n---\n remote.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\nShawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200:\n> I think the above patch is the only thing to do here. Perhaps Alex\n> can write up a formal patch and send it to back to the list and CC\n> Lars Hjemli so he can put it into the patch queue.\n\nhere you go\n\ndiff --git a/remote.c b/remote.c\nindex e7d735b..a25f66d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -882,7 +882,8 @@ int get_fetch_map(struct ref *remote_refs,\n \t\t\t rm->peer_ref->name);\n \t}\n \n-\ttail_link_ref(ref_map, tail);\n+\tif (ref_map)\n+\t\ttail_link_ref(ref_map, tail);\n \n \treturn 0;\n }\n-- \n1.5.3.4.232.ga843\n"},{"id":"55776","messageId":"8c5c35580710141426t2fcd9c3v7cf29168b0fa5a53@mail.gmail.com","threadId":"10207","inReplyTo":"20071012204004.GA6700@steel.home","subject":"Re: [PATCH] Fix a crash in ls-remote when refspec expands into nothing","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2007-10-14T21:26:05Z","receivedAt":"2007-10-14T21:26:05Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 10/12/07, Alex Riesen wrote:\n> Shawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200:\n> > I think the above patch is the only thing to do here. Perhaps Alex\n> > can write up a formal patch and send it to back to the list and CC\n> > Lars Hjemli so he can put it into the patch queue.\n>\n> here you go\n>\n\nThanks. I've replaced the tip of q/vj/fetch-t (in\ngit://hjemli.net/pub/git/git) with this patch.\n\n-- \nlarsh\n"}]}