Bug: Segmentation fault (core dumped)

Hi,

When I am trying to clone an empty repository and I will use together --depth 1 and -b branch_name (branch does not exist) then I get Segmentation fault (repo seems to be cloned correctly).

Please see attachment for more details.
Best regards
Robert Mitwicki

Cloning into '/tmp/coredump.git'...
warning: You appear to have cloned an empty repository. Segmentation fault (core dumped)
Unexpected end of command stream

git version 1.8.4

(gdb) info registers

rax            0x0      0
rbx            0x0      0
rcx            0x0      0
rdx            0x72     114
rsi            0x519d04 5348612
rdi            0x58     88
rbp            0x7d9680 0x7d9680
rsp            0x7fffffffd8f8   0x7fffffffd8f8
r8             0x7fffffffe30b   140737488347915
r9             0x58     88
r10            0x7fffffffd6c0   140737488344768
r11            0x7ffff72c1d50   140737340251472
r12            0x0      0
r13            0x1      1
r14            0x58     88
r15            0x0      0
rip            0x7ffff72c1d68   0x7ffff72c1d68
eflags         0x10206  [ PF IF RF ]
cs             0x33     51
ss             0x2b     43
ds             0x0      0
es             0x0      0
fs             0x0      0
gs             0x0      0

(gdb) bt

#0  0x00007ffff72c1d68 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00000000004204a0 in ?? ()
#2  0x0000000000405b84 in ?? ()
#3  0x0000000000404f6d in ?? ()
#4  0x00007ffff71af76d in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6
#5  0x00000000004053b9 in ?? ()
#6  0x00007fffffffe078 in ?? ()
#7  0x000000000000001c in ?? ()
#8  0x0000000000000008 in ?? ()
#9  0x00007fffffffe2eb in ?? ()

#10 0x00007fffffffe310 in ?? ()
#11 0x00007fffffffe337 in ?? ()
#12 0x0000000000000000 in ?? ()

(gdb) x/60x $sp

0x7fffffffd8f8: 0x00000000      0x00000000      0x007d9680      0x00000000
0x7fffffffd908: 0x00000000      0x00000000      0x00000001      0x00000000
0x7fffffffd918: 0x00000058      0x00000000      0x00000000      0x00000000
0x7fffffffd928: 0x004204a0      0x00000000      0xffffe30b      0x00007fff
0x7fffffffd938: 0xf7fdcd20      0x00007fff      0x007d1fd0      0x00000000
0x7fffffffd948: 0x00000000      0x00000000      0xffffdd60      0x00007fff
0x7fffffffd958: 0xffffdad0      0x00000000      0x00000000      0x00000000
0x7fffffffd968: 0x00000000      0x00000000      0x00000000      0x00000000
0x7fffffffd978: 0xf7fdb630      0x00007fff      0x00000001      0x00007fff
0x7fffffffd988: 0x00000000      0x00000000      0x00000001      0xffffefbd
0x7fffffffd998: 0xf7fdc9c8      0x00007fff      0x003b4700      0x00000000
0x7fffffffd9a8: 0x003b4700      0x00000000      0x00003900      0x00000000
0x7fffffffd9b8: 0xffffddd0      0x00007fff      0xffffde50      0x00007fff
0x7fffffffd9c8: 0xf7ffa4c8      0x00007fff      0x00000000      0x00000000
0x7fffffffd9d8: 0xf7fdcd20      0x00007fff      0xffffdb30      0x00000001

I think we should emit a warning additionally?

---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0aff974..b764ad0 100644
@@ -688,7 +688,7 @@ static void write_refspec_config(const char* src_ref_prefix,
 
 	if (option_mirror || !option_bare) {
 		if (option_single_branch && !option_mirror) {
-			if (option_branch) {
+			if (option_branch && our_head_points_at) {
 				if (strstr(our_head_points_at->name, "refs/tags/"))
 					strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
 						our_head_points_at->name);

1.8.4.1.469.gb38b9db

I think it's nice to credit Robert for reporting the fault in the commit message (something like "reported-by:" or "noticed-by:"...)

Duy

I'll do so in a resend.

Looking at the code a few lines below, this comment comes up:

/*
* otherwise, the next "git fetch" will
* simply fetch from HEAD without updating
* any remote-tracking branch, which is what
* we want.
*/

This behavior was good for the case (!option_branch && !remote_head_points_at) Now we extend that behavior doing nothing to

	 ((!option_branch || !our_head_points_at) &&  !remote_head_points_at)

I am not sure how to handle that case best. The user has given a non existing branch, so it doesn't make sense to track that branch, but only have that registered as a remote*.fetch?

Reading the documentation enhancements of 31b808a
(2012-09-20, clone --single: limit the fetch refspec to fetched branch), doesn't talk about this corner case. So maybe the remote.*.fetch shall be set, but no branch should be checked out, when running
git clone --depth 1 -b test https://github.com/mitfik/coredump.git /tmp/coredump.git

Does that make sense?

Stefan

Looking further back to 86ac751 (Allow cloning an empty repository - 2009-01-23), the reason to allow cloning an empty repository is so that the user does not have do manual configuration, so I agree with your "maybe". git-clone.txt should have a short description about this too in case somebody runs into this and cares enough to check the document before heading to Stack Overflow.

Duy

Actually I only wanted to change one line to prevent a crash, when you specify a non existing branch when cloning:
- if (option_branch) {
+ if (option_branch && our_head_points_at) {

However it turns out this is not a good idea as we still want to setup 'remote.*.fetch', which previously depended the string buffer 'value' being non empty.
Therefore I added a local variable 'set_remote', which determines whether we want to setup 'remote.*.fetch'.

While staring at the code, I also think it is a good idea to restructure the if clauses a little as previously we had

	if (option_mirror || !option_bare) {
		if (option_single_branch && !option_mirror) {

The 'option_mirror' is part of both ifs, but opposing each other. This is not yet done in this patch, as it still needs some thinking how to remove the nesting of the if clauses in a nice way.

---
 builtin/clone.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

index 0aff974..8b9a78a 100644
@@ -686,40 +686,46 @@ static void write_refspec_config(const char* src_ref_prefix,
 	struct strbuf key = STRBUF_INIT;
 	struct strbuf value = STRBUF_INIT;
 
+	int set_remote = 0;
 	if (option_mirror || !option_bare) {
+		set_remote = 1;
 		if (option_single_branch && !option_mirror) {
 			if (option_branch) {
-				if (strstr(our_head_points_at->name, "refs/tags/"))
-					strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
-						our_head_points_at->name);
-				else
-					strbuf_addf(&value, "+%s:%s%s", our_head_points_at->name,
-						branch_top->buf, option_branch);
+				if (our_head_points_at) {
+					if (strstr(our_head_points_at->name, "refs/tags/"))
+						strbuf_addf(&value, "+%s:%s", our_head_points_at->name,
+							our_head_points_at->name);
+					else
+						strbuf_addf(&value, "+%s:%s%s", our_head_points_at->name,
+							branch_top->buf, option_branch);
+				}
 			} else if (remote_head_points_at) {
 				strbuf_addf(&value, "+%s:%s%s", remote_head_points_at->name,
 						branch_top->buf,
 						skip_prefix(remote_head_points_at->name, "refs/heads/"));
+			} else {
+				/*
+				 * otherwise, the next "git fetch" will
+				 * simply fetch from HEAD without updating
+				 * any remote-tracking branch, which is what
+				 * we want.
+				 */
+				set_remote = 0;
 			}
-			/*
-			 * otherwise, the next "git fetch" will
-			 * simply fetch from HEAD without updating
-			 * any remote-tracking branch, which is what
-			 * we want.
-			 */
 		} else {
 			strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top->buf);
 		}
-		/* Configure the remote */
-		if (value.len) {
-			strbuf_addf(&key, "remote.%s.fetch", option_origin);
-			git_config_set_multivar(key.buf, value.buf, "^$", 0);
-			strbuf_reset(&key);
+	}
+	/* Configure the remote */
+	if (set_remote) {
+		strbuf_addf(&key, "remote.%s.fetch", option_origin);
+		git_config_set_multivar(key.buf, value.buf, "^$", 0);
+		strbuf_reset(&key);
 
-			if (option_mirror) {
-				strbuf_addf(&key, "remote.%s.mirror", option_origin);
-				git_config_set(key.buf, "true");
-				strbuf_reset(&key);
-			}
+		if (option_mirror) {
+			strbuf_addf(&key, "remote.%s.mirror", option_origin);
+			git_config_set(key.buf, "true");
+			strbuf_reset(&key);
 		}
 	}
 

1.8.4.1.469.gb38b9db

This segfault only happens when cloning an empty repository and only with option "--single-branch". Or do I miss something?

If we call "git clone" for a non-empty repository with a non-existing branch using "[--single-branch] --branch foo" then Git will abort with a message that the branch doesn't exist in upstream.

In an empty upstream repo the branch doesn't exist, either. So why not abort with
the same message? That would be consistent. Otherwise I'd just override the options
"--single-branch" and "--branch" to "not set".

Ralf

Since 920b691 (clone: refuse to clone if --branch points to bogus ref) we refuse to clone with option "-b" if the specified branch does not exist in the (non-empty) upstream. If the upstream repository is empty, the branch doesn't exist, either. So refuse the clone too.

---
 builtin/clone.c         | 4 ++++
 t/t5706-clone-branch.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index ca3eb68..5af386e 100644
@@ -945,6 +945,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			our_head_points_at = remote_head_points_at;
 	}
 	else {
+		if (option_branch)
+			die(_("Remote branch %s not found in upstream %s"),
+					option_branch, option_origin);
+
 		warning(_("You appear to have cloned an empty repository."));
 		mapped_refs = NULL;
 		our_head_points_at = NULL;

index 56be67e..6e7a7be 100755
@@ -20,7 +20,9 @@ test_expect_success 'setup' '
 	 echo one >file && git add file && git commit -m one &&
 	 git checkout -b two &&
 	 echo two >file && git add file && git commit -m two &&
-	 git checkout master)
+	 git checkout master) &&
+	mkdir empty &&
+	(cd empty && git init)
 '
 
 test_expect_success 'vanilla clone chooses HEAD' '

@@ -61,4 +63,8 @@ test_expect_success 'clone -b with bogus branch' '
 	test_must_fail git clone -b bogus parent clone-bogus
 '
 
+test_expect_success 'clone -b not allowed with empty repos' '
+	test_must_fail git clone -b branch empty clone-branch-empty
+'
+
 test_done

1.8.4.652.g0d6e0ce

Yeah, much simpler approach :)

Duy