threads / patch / 10207

patchFixed crash in fetching remote tags when there is not tags.

Subject: [PATCH] Fixed crash in fetching remote tags when there is not tags.

## tl;dr

10 messages between Oct 9, 2007 and Oct 14, 2007. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Väinö Järvelä· Oct 9, 2007, 08:51 UTC · lore

[PATCH] Added a test for fetching remote tags when there is not tags.

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ä <v@pp.inet.fi>
---
 t/t5510-fetch.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Show changes to t/t5510-fetch.sh +12 −0

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ä· Oct 9, 2007, 08:51 UTC · re: Väinö Järvelä · lore

Signed-off-by: Väinö Järvelä <v@pp.inet.fi>
---
 remote.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Show changes to remote.c +2 −0

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ä· Oct 9, 2007, 10:52 UTC · re: Väinö Järvelä · lore

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

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:

Show 26 quoted lines

> Signed-off-by: Väinö Järvelä <v@pp.inet.fi>
> ---
>  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· Oct 9, 2007, 18:20 UTC · re: Väinö Järvelä · lore

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

Väinö Järvelä, Tue, Oct 09, 2007 12:52:01 +0200:

Show 8 quoted lines

> 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· Oct 10, 2007, 05:10 UTC · re: Alex Riesen · lore

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

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· Oct 10, 2007, 21:27 UTC · re: Jeff King · lore

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

Jeff King, Wed, Oct 10, 2007 07:10:35 +0200:

Show 9 quoted lines

> 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.

Show changes to remote.c +2 −1

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· Oct 10, 2007, 21:33 UTC · re: Alex Riesen · lore

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

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· Oct 12, 2007, 04:07 UTC · re: Alex Riesen · lore

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

Alex Riesen <raa.lkml@gmail.com> wrote:

Show 23 quoted lines

> 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 <hjemli@gmail.com> so he can put it into the patch queue.

-- 
Shawn.

Alex Riesen· Oct 12, 2007, 20:40 UTC · re: Shawn O. Pearce · lore

[PATCH] Fix a crash in ls-remote when refspec expands into nothing

Originally-by: Väinö Järvelä <v@pp.inet.fi>
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 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 <hjemli@gmail.com> so he can put it into the patch queue.

here you go

Show changes to remote.c +2 −1

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· Oct 14, 2007, 21:26 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix a crash in ls-remote when refspec expands into nothing

On 10/12/07, Alex Riesen <raa.lkml@gmail.com> wrote:

Show 7 quoted lines

> 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 <hjemli@gmail.com> 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