Bug: Segmentation fault (core dumped)

Bug: Segmentation fault (core dumped)

[Robert Mitwicki]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]

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

[-- Attachment #2: Log.txt --]
[-- Type: text/plain, Size: 2727 bytes --]

>  git clone --depth 1 -b test https://github.com/mitfik/coredump.git /tmp/coredump.git
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
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

[PATCH] clone: do not segfault when specifying a nonexistent branch

[Stefan Beller]

I think we should emit a warning additionally?

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0aff974..b764ad0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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

Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Duy Nguyen]

On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
> I think we should emit a warning additionally?
>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>

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

> ---
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0aff974..b764ad0 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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);

This prevents the segfault, but what about remote.*.fetch? Should we
setup standard refspec for fetch or..?
-- 
Duy

Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Stefan Beller]

On 10/05/2013 01:55 AM, Duy Nguyen wrote:
> On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
> <stefanbeller@googlemail.com> wrote:
>> I think we should emit a warning additionally?
>>
>> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> 
> I think it's nice to credit Robert for reporting the fault in the
> commit message (something like "reported-by:" or "noticed-by:"...)

I'll do so in a resend.

> 
>> ---
>>  builtin/clone.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 0aff974..b764ad0 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -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);
> 
> This prevents the segfault, but what about remote.*.fetch? Should we
> setup standard refspec for fetch or..?
> 

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

Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Duy Nguyen]

On Sun, Oct 6, 2013 at 4:27 PM, Stefan Beller
<stefanbeller@googlemail.com> wrote:
>>> @@ -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);
>>
>> This prevents the segfault, but what about remote.*.fetch? Should we
>> setup standard refspec for fetch or..?
>>
>
> 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?

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

[PATCH] clone: do not segfault when specifying a nonexistent branch

[Stefan Beller]

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.

Reported-by: Robert Mitwicki <robert.mitwicki@opensoftware.pl>
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/clone.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0aff974..8b9a78a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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

Re: [PATCH] clone: do not segfault when specifying a nonexistent branch

[Ralf Thielow]

On Sat, Oct 5, 2013 at 1:55 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, Oct 4, 2013 at 9:20 PM, Stefan Beller
> <stefanbeller@googlemail.com> wrote:
>> I think we should emit a warning additionally?
>>
>> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
>
> I think it's nice to credit Robert for reporting the fault in the
> commit message (something like "reported-by:" or "noticed-by:"...)
>
>> ---
>>  builtin/clone.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 0aff974..b764ad0 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -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);
>
> This prevents the segfault, but what about remote.*.fetch? Should we
> setup standard refspec for fetch or..?
> --
> Duy

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

[PATCH] clone --branch: refuse to clone if upstream repo is empty

[Ralf Thielow]

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.

Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
 builtin/clone.c         | 4 ++++
 t/t5706-clone-branch.sh | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index ca3eb68..5af386e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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;
diff --git a/t/t5706-clone-branch.sh b/t/t5706-clone-branch.sh
index 56be67e..6e7a7be 100755
--- a/t/t5706-clone-branch.sh
+++ b/t/t5706-clone-branch.sh
@@ -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

Re: [PATCH] clone --branch: refuse to clone if upstream repo is empty

[Duy Nguyen]

On Fri, Oct 11, 2013 at 11:49 PM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
> 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.

Yeah, much simpler approach :)

> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
>  builtin/clone.c         | 4 ++++
>  t/t5706-clone-branch.sh | 8 +++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ca3eb68..5af386e 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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;
> diff --git a/t/t5706-clone-branch.sh b/t/t5706-clone-branch.sh
> index 56be67e..6e7a7be 100755
> --- a/t/t5706-clone-branch.sh
> +++ b/t/t5706-clone-branch.sh
> @@ -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
>



-- 
Duy