Stefan Sperling <stsp@stsp.name> writes:

> On Thu, Sep 09, 2021 at 03:12:49PM +0200, Omar Polo wrote:
>> Hello,
>> 
>> I've spotted a segfault during `got fetch -d'
>> 
>> In got.c:2040
>> 
>>    2039                 TAILQ_FOREACH(pe, their_refs, entry) {
>>    2040                         if (strcmp(local_refname, pe->path) == 0)
>>    2041                                 break;
>>    2042                 }
>> 
>> local_refname is NULL and strcmp crashes.  I couldn't get a
>> corefile thought
>> 
>> --8<---------------cut here---------------start------------->8---
>> % egdb got
>> (gdb) r fetch -d
>> Starting program: /home/op/bin/got fetch -d
>> [New process 79176]
>> Connecting to "origin" git@omarpolo.com
>> Already up-to-date
>> 
>> Thread 1 received signal SIGSEGV, Segmentation fault.
>> strcmp () at /usr/src/lib/libc/arch/amd64/string/strcmp.S:45
>> 45              movq    8(%rdi),%rax
>> (gdb) bt
>> #0  strcmp () at /usr/src/lib/libc/arch/amd64/string/strcmp.S:45
>> #1  0x000006cbf275413a in delete_missing_refs (their_refs=0x7f7ffffecf00,
>>     their_symrefs=0x7f7ffffecef0, remote=0x6cdfe936250, verbosity=0,
>>     repo=0x6cdfe917070) at got.c:2040
>> #2  0x000006cbf274475c in cmd_fetch (argc=0, argv=0x7f7ffffed0a0) at got.c:2538
>> #3  0x000006cbf274105f in main (argc=2, argv=0x7f7ffffed090) at got.c:252
>> (gdb) f 1
>> #1  0x000006cbf275413a in delete_missing_refs (their_refs=0x7f7ffffecf00,
>>     their_symrefs=0x7f7ffffecef0, remote=0x6cdfe936250, verbosity=0,
>>     repo=0x6cdfe917070) at got.c:2040
>> 2040                            if (strcmp(local_refname, pe->path) == 0)
>> (gdb) p local_refname
>> $1 = 0x0
>> --8<---------------cut here---------------end--------------->8---
>> 
>> A plain `got fetch' works fine.
>> 
>> I'm using got compiled from the latest commit 9b21d88
>> 
>> Background: (probably not interesting but who knows)
>> 
>> I've deleted a tag server side with `git tag -d 1.0' followed by a `git
>> gc', then I wanted to pull the change.  After a `got fetch' the tag 1.0
>> is still there, so I tried with -d and found the crash.
>> 
>> Here's the repo config in case it's useful:
>> 
>> 	% cat /home/op/git/vc-got.git/config
>> 	[core]
>> 		repositoryformatversion = 0
>> 		filemode = true
>> 		bare = true
>> 	[remote "origin"]
>> 		url = ssh://git@omarpolo.com/vc-got.git
>> 		fetch = +refs/*:refs/*
>> 		mirror = true
>> 	[remote "github"]
>> 		url = ssh://git@github.com/omar-polo/vc-got
>> 		fetch = +refs/heads/*:refs/remotes/github/*
>> 
>> Thanks,
>> 
>> Omar Polo
>> 
>> 
>
> Thank you, Omar.
>
> The patch below fixes the bug and adds a test which reproduces the crash.
> Does this work for you?


With this I can't trigger the crash anymore, thanks!


> diff 678d8c1fe20f1eab5a3568135b24f00c9d22c33c /home/stsp/src/got
> blob - 5f3984d14029304bc35fa0a2ae1caeff8de74a69
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -2020,8 +2020,11 @@ delete_missing_refs(struct got_pathlist_head *their_re
>  
>  	TAILQ_FOREACH(re, &my_refs, entry) {
>  		const char *refname = got_ref_get_name(re->ref);
> +		const char *their_refname;
>  
> -		if (!remote->mirror_references) {
> +		if (remote->mirror_references) {
> +			their_refname = refname;
> +		} else {
>  			if (strncmp(refname, remote_namespace,
>  			    strlen(remote_namespace)) == 0) {
>  				if (strcmp(refname + strlen(remote_namespace),
> @@ -2034,17 +2037,19 @@ delete_missing_refs(struct got_pathlist_head *their_re
>  				}
>  			} else if (strncmp(refname, "refs/tags/", 10) != 0)
>  				continue;
> -		}
>  
> +			their_refname = local_refname;
> +		}
> +	
>  		TAILQ_FOREACH(pe, their_refs, entry) {
> -			if (strcmp(local_refname, pe->path) == 0)
> +			if (strcmp(their_refname, pe->path) == 0)
>  				break;
>  		}
>  		if (pe != NULL)
>  			continue;
>  
>  		TAILQ_FOREACH(pe, their_symrefs, entry) {
> -			if (strcmp(local_refname, pe->path) == 0)
> +			if (strcmp(their_refname, pe->path) == 0)
>  				break;
>  		}
>  		if (pe != NULL)
> blob - e0587383f411d35c4be341e74b562fba63fbfbf8
> file + regress/cmdline/fetch.sh
> --- regress/cmdline/fetch.sh
> +++ regress/cmdline/fetch.sh
> @@ -509,6 +509,102 @@ test_fetch_delete_branch() {
>  
>  }
>  
> +test_fetch_delete_branch_mirror() {
> +	local testroot=`test_init fetch_delete_branch_mirror`
> +	local testurl=ssh://127.0.0.1/$testroot
> +	local commit_id=`git_show_head $testroot/repo`
> +
> +	got branch -r $testroot/repo -c $commit_id foo
> +	got ref -r $testroot/repo -c $commit_id refs/hoo/boo/zoo
> +	got tag -r $testroot/repo -c $commit_id -m tag "1.0" >/dev/null
> +	local tag_id=`got ref -r $testroot/repo -l \
> +		| grep "^refs/tags/$tag" | tr -d ' ' | cut -d: -f2`
> +
> +	got clone -a -m -q $testurl/repo $testroot/repo-clone
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		echo "got clone command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got ref -l -r $testroot/repo-clone > $testroot/stdout
> +
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/foo: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	# refs/hoo/boo/zoo is missing because it is outside of refs/heads
> +	echo "refs/tags/1.0: $tag_id" >> $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got branch -r $testroot/repo -d foo >/dev/null
> +
> +	got fetch -q -r $testroot/repo-clone
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		echo "got fetch command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got ref -l -r $testroot/repo-clone > $testroot/stdout
> +
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	echo "refs/heads/foo: $commit_id" >> $testroot/stdout.expected
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	# refs/hoo/boo/zoo is missing because it is outside of refs/heads
> +	echo "refs/tags/1.0: $tag_id" >> $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got fetch -d -q -r $testroot/repo-clone > $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		echo "got fetch command failed unexpectedly" >&2
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo -n > $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	got ref -l -r $testroot/repo-clone > $testroot/stdout
> +
> +	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
> +	# refs/heads/foo is now deleted
> +	echo "refs/heads/master: $commit_id" >> $testroot/stdout.expected
> +	# refs/hoo/boo/zoo is missing because it is outside of refs/heads
> +	echo "refs/tags/1.0: $tag_id" >> $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout $testroot/stdout.expected
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
> +
> +}
> +
>  test_fetch_update_tag() {
>  	local testroot=`test_init fetch_update_tag`
>  	local testurl=ssh://127.0.0.1/$testroot
> @@ -1191,6 +1287,7 @@ run_test test_fetch_branch
>  run_test test_fetch_all
>  run_test test_fetch_empty_packfile
>  run_test test_fetch_delete_branch
> +run_test test_fetch_delete_branch_mirror
>  run_test test_fetch_update_tag
>  run_test test_fetch_reference
>  run_test test_fetch_replace_symref