# [PATCH] Fixed crash in fetching remote tags when there is not tags. 10 messages from 2007-10-09 to 2007-10-14. Participants: Väinö Järvelä, Alex Riesen, Jeff King, Shawn O. Pearce, Lars Hjemli. Thread: https://gitlist.dev/t/10207 ## Väinö Järvelä, 2007-10-09 08:51 Subject: [PATCH] Added a test for fetching remote tags when there is not tags. Message-ID: <1191919868-4963-1-git-send-email-v@pp.inet.fi> URL: https://gitlist.dev/e/1191919868-4963-1-git-send-email-v%40pp.inet.fi ``` When a user runs "git fetch -t", git crashes when it doesn't find any tags on the remote repository. Signed-off-by: Väinö Järvelä --- t/t5510-fetch.sh | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 73a4e3c..40ebf2e 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -67,6 +67,18 @@ test_expect_success "fetch test for-merge" ' cut -f -2 .git/FETCH_HEAD >actual && diff expected actual' +test_expect_success 'fetch tags when there is no tags' ' + + cd "$D" && + + mkdir notags && + cd notags && + git init && + + git fetch -t .. + +' + test_expect_success 'fetch following tags' ' cd "$D" && -- 1.5.3.4.1156.g5407-dirty ``` ## Väinö Järvelä, 2007-10-09 08:51 Subject: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <1191919868-4963-2-git-send-email-v@pp.inet.fi> URL: https://gitlist.dev/e/1191919868-4963-2-git-send-email-v%40pp.inet.fi In-Reply-To: <1191919868-4963-1-git-send-email-v@pp.inet.fi> ``` Signed-off-by: Väinö Järvelä --- remote.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/remote.c b/remote.c index e7d735b..5e92378 100644 --- a/remote.c +++ b/remote.c @@ -537,6 +537,8 @@ static int count_refspec_match(const char *pattern, static void tail_link_ref(struct ref *ref, struct ref ***tail) { + if (!ref) return; + **tail = ref; while (ref->next) ref = ref->next; -- 1.5.3.4.1156.g5407-dirty ``` ## Väinö Järvelä, 2007-10-09 10:52 Subject: Re: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <81553116-3A4F-4526-A772-9A43C53D3E22@pp.inet.fi> URL: https://gitlist.dev/e/81553116-3A4F-4526-A772-9A43C53D3E22%40pp.inet.fi In-Reply-To: <1191919868-4963-2-git-send-email-v@pp.inet.fi> ``` Hi, These patches should have been numbered, sorry, The test patch was supposed be number 1 of 2. The patches were done on top of next. The fix should be checked, as it's mostly a quick fix to get it to work, probably not the correct way to fix that bug, or is it? -- Väinö On Oct 9, 2007, at 11:51, Väinö Järvelä wrote: > Signed-off-by: Väinö Järvelä > --- > remote.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/remote.c b/remote.c > index e7d735b..5e92378 100644 > --- a/remote.c > +++ b/remote.c > @@ -537,6 +537,8 @@ static int count_refspec_match(const char > *pattern, > > static void tail_link_ref(struct ref *ref, struct ref ***tail) > { > + if (!ref) return; > + > **tail = ref; > while (ref->next) > ref = ref->next; > -- > 1.5.3.4.1156.g5407-dirty > > - > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ``` ## Alex Riesen, 2007-10-09 18:20 Subject: Re: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <20071009182043.GA2997@steel.home> URL: https://gitlist.dev/e/20071009182043.GA2997%40steel.home In-Reply-To: <81553116-3A4F-4526-A772-9A43C53D3E22@pp.inet.fi> ``` Väinö Järvelä, Tue, Oct 09, 2007 12:52:01 +0200: > Hi, > > These patches should have been numbered, sorry, The test patch was > supposed be number 1 of 2. > > The patches were done on top of next. The fix should be checked, as > it's mostly a quick fix to get it to work, probably not the correct > way to fix that bug, or is it? Your test does not crash on my system and your fix is obviously bogus. Just take a look at the only call site of the tail_link_ref: ret cannot be NULL. alloc_ref will crash in memset, if this were the case. If you can reproduce the crash reliably, try compiling git with -g and run it in your test with valgrind or gdb (assuming these are available to you). ``` ## Jeff King, 2007-10-10 05:10 Subject: Re: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <20071010051034.GA30834@coredump.intra.peff.net> URL: https://gitlist.dev/e/20071010051034.GA30834%40coredump.intra.peff.net In-Reply-To: <20071009182043.GA2997@steel.home> ``` On Tue, Oct 09, 2007 at 08:20:43PM +0200, Alex Riesen wrote: > Your test does not crash on my system and your fix is obviously bogus. > Just take a look at the only call site of the tail_link_ref: ret > cannot be NULL. alloc_ref will crash in memset, if this were the case. His work is almost certainly on top of next, which crashes reliably with the test and has an additional call site for tail_link_ref. Aside from some trailing whitespace in the patch, I think his fix is reasonable. -Peff ``` ## Alex Riesen, 2007-10-10 21:27 Subject: Re: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <20071010212735.GB16635@steel.home> URL: https://gitlist.dev/e/20071010212735.GB16635%40steel.home In-Reply-To: <20071010051034.GA30834@coredump.intra.peff.net> ``` Jeff King, Wed, Oct 10, 2007 07:10:35 +0200: > On Tue, Oct 09, 2007 at 08:20:43PM +0200, Alex Riesen wrote: > > > Your test does not crash on my system and your fix is obviously bogus. > > Just take a look at the only call site of the tail_link_ref: ret > > cannot be NULL. alloc_ref will crash in memset, if this were the case. > > His work is almost certainly on top of next, which crashes reliably with > the test and has an additional call site for tail_link_ref. Aside from > some trailing whitespace in the patch, I think his fix is reasonable. Ach, I see. Still, I'd suggest move the test into the caller, firstly because it is the only place that special. Also, I can't think of a proper reason to add a NULL ref to a reflist, and so the crashing tail_link_ref will help us find the callers which use tail_link_ref incorrectly (illogically too). As the result of patter expansion can be NULL (empty pattern, as it seems), lets just check for it. I parked the patch below locally. diff --git a/remote.c b/remote.c index 5e92378..58d63ed 100644 --- a/remote.c +++ b/remote.c @@ -884,7 +884,8 @@ int get_fetch_map(struct ref *remote_refs, rm->peer_ref->name); } - tail_link_ref(ref_map, tail); + if (ref_map) + tail_link_ref(ref_map, tail); return 0; } ``` ## Jeff King, 2007-10-10 21:33 Subject: Re: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <20071010213305.GB8518@sigill.intra.peff.net> URL: https://gitlist.dev/e/20071010213305.GB8518%40sigill.intra.peff.net In-Reply-To: <20071010212735.GB16635@steel.home> ``` On Wed, Oct 10, 2007 at 11:27:35PM +0200, Alex Riesen wrote: > Still, I'd suggest move the test into the caller, firstly because it > is the only place that special. Also, I can't think of a proper reason Yes, I agree with that. I came very close to suggesting it in my other mail, but then realized I had never even looked at the surrounding code, and I ought not to be making assessments of how that data structure should be used. But now there are two of us. :) -Peff ``` ## Shawn O. Pearce, 2007-10-12 04:07 Subject: Re: [PATCH] Fixed crash in fetching remote tags when there is not tags. Message-ID: <20071012040745.GC27899@spearce.org> URL: https://gitlist.dev/e/20071012040745.GC27899%40spearce.org In-Reply-To: <20071010212735.GB16635@steel.home> ``` Alex Riesen wrote: > Still, I'd suggest move the test into the caller, firstly because it > is the only place that special. Also, I can't think of a proper reason > to add a NULL ref to a reflist, and so the crashing tail_link_ref will > help us find the callers which use tail_link_ref incorrectly > (illogically too). > > As the result of patter expansion can be NULL (empty pattern, as it > seems), lets just check for it. I parked the patch below locally. > > diff --git a/remote.c b/remote.c > index 5e92378..58d63ed 100644 > --- a/remote.c > +++ b/remote.c > @@ -884,7 +884,8 @@ int get_fetch_map(struct ref *remote_refs, > rm->peer_ref->name); > } > > - tail_link_ref(ref_map, tail); > + if (ref_map) > + tail_link_ref(ref_map, tail); > > return 0; > } I disagree with Alex's argument above, because the result of the pattern expansion can be NULL due to a pattern that matched nothing from the remote. This can easily happen if the user originally configured "remote.$name.fetch = +refs/heads/subdir/*:..." and then the remote deletes all branches from refs/heads/subdir at some point in the future. I think the above patch is the only thing to do here. Perhaps Alex can write up a formal patch and send it to back to the list and CC Lars Hjemli so he can put it into the patch queue. -- Shawn. ``` ## Alex Riesen, 2007-10-12 20:40 Subject: [PATCH] Fix a crash in ls-remote when refspec expands into nothing Message-ID: <20071012204004.GA6700@steel.home> URL: https://gitlist.dev/e/20071012204004.GA6700%40steel.home In-Reply-To: <20071012040745.GC27899@spearce.org> ``` Originally-by: Väinö Järvelä Signed-off-by: Alex Riesen --- remote.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) Shawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200: > I think the above patch is the only thing to do here. Perhaps Alex > can write up a formal patch and send it to back to the list and CC > Lars Hjemli so he can put it into the patch queue. here you go diff --git a/remote.c b/remote.c index e7d735b..a25f66d 100644 --- a/remote.c +++ b/remote.c @@ -882,7 +882,8 @@ int get_fetch_map(struct ref *remote_refs, rm->peer_ref->name); } - tail_link_ref(ref_map, tail); + if (ref_map) + tail_link_ref(ref_map, tail); return 0; } -- 1.5.3.4.232.ga843 ``` ## Lars Hjemli, 2007-10-14 21:26 Subject: Re: [PATCH] Fix a crash in ls-remote when refspec expands into nothing Message-ID: <8c5c35580710141426t2fcd9c3v7cf29168b0fa5a53@mail.gmail.com> URL: https://gitlist.dev/e/8c5c35580710141426t2fcd9c3v7cf29168b0fa5a53%40mail.gmail.com In-Reply-To: <20071012204004.GA6700@steel.home> ``` On 10/12/07, Alex Riesen wrote: > Shawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200: > > I think the above patch is the only thing to do here. Perhaps Alex > > can write up a formal patch and send it to back to the list and CC > > Lars Hjemli so he can put it into the patch queue. > > here you go > Thanks. I've replaced the tip of q/vj/fetch-t (in git://hjemli.net/pub/git/git) with this patch. -- larsh ```