[PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[André Goddard Rosa]

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

Hi, all!

    Please cc: me as I'm not subscribed. I'm sending the patch inline
only for review, probably it is mangled.
    Please use the attached patch if you agree with it. Sorry about
sending it attached.

>From b2af9e783e7d8974b969c01f7a2de07b9cd5cf70 Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:14:57 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
permission to the repository.

When trying to "git-pull" with my personal user in a tree owned by root,
git was crashing with segmentation fault.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 builtin-fetch--tool.c |   12 ++++++++++--
 builtin-fetch.c       |   14 +++++++++++---
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
 	if (!strcmp("append-fetch-head", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;

 		if (argc != 8)
 			return error("append-fetch-head takes 6 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = append_fetch_head(fp, argv[2], argv[3],
 					   argv[4], argv[5],
 					   argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
 	if (!strcmp("native-store", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;

 		if (argc != 5)
 			return error("fetch-native-store takes 3 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = fetch_native_store(fp, argv[2], argv[3], argv[4],
 					    verbose, force);
 		fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..5909d2f 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -263,8 +263,11 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
+	char *filename = git_path("FETCH_HEAD");

-	fp = fopen(git_path("FETCH_HEAD"), "a");
+	fp = fopen(filename, "a");
+	if (!fp)
+		return error("cannot open %s: %s\n", filename, strerror(errno));
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;

@@ -487,8 +490,13 @@ static int do_fetch(struct transport *transport,
 		die("Don't know how to fetch from %s", transport->url);

 	/* if not appending, truncate FETCH_HEAD */
-	if (!append)
-		fclose(fopen(git_path("FETCH_HEAD"), "w"));
+	if (!append) {
+		char *filename = git_path("FETCH_HEAD");
+		int fd = fopen(filename, "w");
+		if (!fd)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
+		fclose(fd);
+	}

 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);

-- 
1.5.3.6.861.gd794-dirty

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch --]
[-- Type: text/x-patch; name=0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch, Size: 2916 bytes --]

From b2af9e783e7d8974b969c01f7a2de07b9cd5cf70 Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:14:57 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

When trying to "git-pull" with my personal user in a tree owned by root,
git was crashing with segmentation fault.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 builtin-fetch--tool.c |   12 ++++++++++--
 builtin-fetch.c       |   14 +++++++++++---
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 	if (!strcmp("append-fetch-head", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;
 
 		if (argc != 8)
 			return error("append-fetch-head takes 6 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = append_fetch_head(fp, argv[2], argv[3],
 					   argv[4], argv[5],
 					   argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 	if (!strcmp("native-store", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;
 
 		if (argc != 5)
 			return error("fetch-native-store takes 3 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = fetch_native_store(fp, argv[2], argv[3], argv[4],
 					    verbose, force);
 		fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..5909d2f 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -263,8 +263,11 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
+	char *filename = git_path("FETCH_HEAD");
 
-	fp = fopen(git_path("FETCH_HEAD"), "a");
+	fp = fopen(filename, "a");
+	if (!fp)
+		return error("cannot open %s: %s\n", filename, strerror(errno));
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;
 
@@ -487,8 +490,13 @@ static int do_fetch(struct transport *transport,
 		die("Don't know how to fetch from %s", transport->url);
 
 	/* if not appending, truncate FETCH_HEAD */
-	if (!append)
-		fclose(fopen(git_path("FETCH_HEAD"), "w"));
+	if (!append) {
+		char *filename = git_path("FETCH_HEAD");
+		int fd = fopen(filename, "w");
+		if (!fd)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
+		fclose(fd);
+	}
 
 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
 
-- 
1.5.3.6.861.gd794-dirty

Re: [PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[Alex Riesen]

André Goddard Rosa, Thu, Nov 22, 2007 01:59:00 +0100:
> @@ -487,8 +490,13 @@ static int do_fetch(struct transport *transport,
>  		die("Don't know how to fetch from %s", transport->url);
>  
>  	/* if not appending, truncate FETCH_HEAD */
> -	if (!append)
> -		fclose(fopen(git_path("FETCH_HEAD"), "w"));
> +	if (!append) {
> +		char *filename = git_path("FETCH_HEAD");
> +		int fd = fopen(filename, "w");

This should have been "FILE *fp", not "int fd".

> +		if (!fd)
> +			return error("cannot open %s: %s\n", filename, strerror(errno));
> +		fclose(fd);
> +	}
>  
>  	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
>  
> -- 
> 1.5.3.6.861.gd794-dirty
> 

Re: [PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[André Goddard Rosa]

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

On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> André Goddard Rosa, Thu, Nov 22, 2007 01:59:00 +0100:
> > @@ -487,8 +490,13 @@ static int do_fetch(struct transport *transport,
> >               die("Don't know how to fetch from %s", transport->url);
> >
> >       /* if not appending, truncate FETCH_HEAD */
> > -     if (!append)
> > -             fclose(fopen(git_path("FETCH_HEAD"), "w"));
> > +     if (!append) {
> > +             char *filename = git_path("FETCH_HEAD");
> > +             int fd = fopen(filename, "w");
>
> This should have been "FILE *fp", not "int fd".
>

Hi, Alex!

Many thanks, you're right.

I tested it here before posting but luckly (or not, as I didn't catch
this when compiling) it worked,
as a pointer have the sizeof(int) in my x86 platform. >:|

Would you please comment on the attached patch and see if it's ok?

From dbadc5213b9957fb575c6da8528e5dd7a3f1f43e Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Andr=C3=A9=20Goddard=20Rosa?= <andre.goddard@gmail.com>
Date: Thu, 22 Nov 2007 20:22:23 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
 permission to the repository.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 builtin-fetch--tool.c |   12 ++++++++++--
 builtin-fetch.c       |   21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
 	if (!strcmp("append-fetch-head", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;

 		if (argc != 8)
 			return error("append-fetch-head takes 6 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = append_fetch_head(fp, argv[2], argv[3],
 					   argv[4], argv[5],
 					   argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
 	if (!strcmp("native-store", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;

 		if (argc != 5)
 			return error("fetch-native-store takes 3 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = fetch_native_store(fp, argv[2], argv[3], argv[4],
 					    verbose, force);
 		fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..84c8ed4 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -255,7 +255,7 @@ static int update_local_ref(struct ref *ref,
 	}
 }

-static void store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, struct ref *ref_map)
 {
 	FILE *fp;
 	struct commit *commit;
@@ -263,8 +263,13 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
+	char *filename = git_path("FETCH_HEAD");

-	fp = fopen(git_path("FETCH_HEAD"), "a");
+	fp = fopen(filename, "a");
+	if (!fp) {
+		error("cannot open %s: %s\n", filename, strerror(errno));
+		return 1;
+	}
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;

@@ -335,6 +340,7 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
 		}
 	}
 	fclose(fp);
+	return 0;
 }

 /*
@@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport,
struct ref *ref_map)
 	if (ret)
 		ret = transport_fetch_refs(transport, ref_map);
 	if (!ret)
-		store_updated_refs(transport->url, ref_map);
+		ret |= store_updated_refs(transport->url, ref_map);
 	transport_unlock_pack(transport);
 	return ret;
 }
@@ -487,8 +493,13 @@ static int do_fetch(struct transport *transport,
 		die("Don't know how to fetch from %s", transport->url);

 	/* if not appending, truncate FETCH_HEAD */
-	if (!append)
-		fclose(fopen(git_path("FETCH_HEAD"), "w"));
+	if (!append) {
+		char *filename = git_path("FETCH_HEAD");
+		FILE *fp = fopen(filename, "w");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
+		fclose(fp);
+	}

 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);

-- 
1.5.3.6.861.gd794-dirty

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch --]
[-- Type: text/x-patch; name=0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch, Size: 3642 bytes --]

From dbadc5213b9957fb575c6da8528e5dd7a3f1f43e Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Andr=C3=A9=20Goddard=20Rosa?= <andre.goddard@gmail.com>
Date: Thu, 22 Nov 2007 20:22:23 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
 permission to the repository.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 builtin-fetch--tool.c |   12 ++++++++++--
 builtin-fetch.c       |   21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 	if (!strcmp("append-fetch-head", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;
 
 		if (argc != 8)
 			return error("append-fetch-head takes 6 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = append_fetch_head(fp, argv[2], argv[3],
 					   argv[4], argv[5],
 					   argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 	if (!strcmp("native-store", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;
 
 		if (argc != 5)
 			return error("fetch-native-store takes 3 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = fetch_native_store(fp, argv[2], argv[3], argv[4],
 					    verbose, force);
 		fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..84c8ed4 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -255,7 +255,7 @@ static int update_local_ref(struct ref *ref,
 	}
 }
 
-static void store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, struct ref *ref_map)
 {
 	FILE *fp;
 	struct commit *commit;
@@ -263,8 +263,13 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
+	char *filename = git_path("FETCH_HEAD");
 
-	fp = fopen(git_path("FETCH_HEAD"), "a");
+	fp = fopen(filename, "a");
+	if (!fp) {
+		error("cannot open %s: %s\n", filename, strerror(errno));
+		return 1;
+	}
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;
 
@@ -335,6 +340,7 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
 		}
 	}
 	fclose(fp);
+	return 0;
 }
 
 /*
@@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	if (ret)
 		ret = transport_fetch_refs(transport, ref_map);
 	if (!ret)
-		store_updated_refs(transport->url, ref_map);
+		ret |= store_updated_refs(transport->url, ref_map);
 	transport_unlock_pack(transport);
 	return ret;
 }
@@ -487,8 +493,13 @@ static int do_fetch(struct transport *transport,
 		die("Don't know how to fetch from %s", transport->url);
 
 	/* if not appending, truncate FETCH_HEAD */
-	if (!append)
-		fclose(fopen(git_path("FETCH_HEAD"), "w"));
+	if (!append) {
+		char *filename = git_path("FETCH_HEAD");
+		FILE *fp = fopen(filename, "w");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
+		fclose(fp);
+	}
 
 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
 
-- 
1.5.3.6.861.gd794-dirty

[Resend PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[André Goddard Rosa]

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

On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> André Goddard Rosa, Thu, Nov 22, 2007 01:59:00 +0100:
> > @@ -487,8 +490,13 @@ static int do_fetch(struct transport *transport,
> >               die("Don't know how to fetch from %s", transport->url);
> >
> >       /* if not appending, truncate FETCH_HEAD */
> > -     if (!append)
> > -             fclose(fopen(git_path("FETCH_HEAD"), "w"));
> > +     if (!append) {
> > +             char *filename = git_path("FETCH_HEAD");
> > +             int fd = fopen(filename, "w");
>
> This should have been "FILE *fp", not "int fd".
>

Hi, Alex!

Many thanks, you're right.

I tested it here before posting but luckly (or not, as I didn't catch
this when compiling) it worked,
as a pointer have the sizeof(int) in my x86 platform. >:|

Would you please comment on the attached patch and see if it's ok?

From dbadc5213b9957fb575c6da8528e5dd7a3f1f43e Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Andr=C3=A9=20Goddard=20Rosa?= <andre.goddard@gmail.com>
Date: Thu, 22 Nov 2007 20:22:23 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
 permission to the repository.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 builtin-fetch--tool.c |   12 ++++++++++--
 builtin-fetch.c       |   21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
        if (!strcmp("append-fetch-head", argv[1])) {
                int result;
                FILE *fp;
+               char *filename;

                if (argc != 8)
                        return error("append-fetch-head takes 6 args");
-               fp = fopen(git_path("FETCH_HEAD"), "a");
+               filename = git_path("FETCH_HEAD");
+               fp = fopen(filename, "a");
+               if (!fp)
+                       return error("cannot open %s: %s\n", filename,
strerror(errno));
                result = append_fetch_head(fp, argv[2], argv[3],
                                           argv[4], argv[5],
                                           argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
        if (!strcmp("native-store", argv[1])) {
                int result;
                FILE *fp;
+               char *filename;

                if (argc != 5)
                        return error("fetch-native-store takes 3 args");
-               fp = fopen(git_path("FETCH_HEAD"), "a");
+               filename = git_path("FETCH_HEAD");
+               fp = fopen(filename, "a");
+               if (!fp)
+                       return error("cannot open %s: %s\n", filename,
strerror(errno));
                result = fetch_native_store(fp, argv[2], argv[3], argv[4],
                                            verbose, force);
                fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..84c8ed4 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -255,7 +255,7 @@ static int update_local_ref(struct ref *ref,
        }
 }

-static void store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, struct ref *ref_map)
 {
        FILE *fp;
        struct commit *commit;
@@ -263,8 +263,13 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
        char note[1024];
        const char *what, *kind;
        struct ref *rm;
+       char *filename = git_path("FETCH_HEAD");

-       fp = fopen(git_path("FETCH_HEAD"), "a");
+       fp = fopen(filename, "a");
+       if (!fp) {
+               error("cannot open %s: %s\n", filename, strerror(errno));
+               return 1;
+       }
        for (rm = ref_map; rm; rm = rm->next) {
                struct ref *ref = NULL;

@@ -335,6 +340,7 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
                }
        }
        fclose(fp);
+       return 0;
 }

 /*
@@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport,
struct ref *ref_map)
        if (ret)
                ret = transport_fetch_refs(transport, ref_map);
        if (!ret)
-               store_updated_refs(transport->url, ref_map);
+               ret |= store_updated_refs(transport->url, ref_map);
        transport_unlock_pack(transport);
        return ret;
 }
@@ -487,8 +493,13 @@ static int do_fetch(struct transport *transport,
                die("Don't know how to fetch from %s", transport->url);

        /* if not appending, truncate FETCH_HEAD */
-       if (!append)
-               fclose(fopen(git_path("FETCH_HEAD"), "w"));
+       if (!append) {
+               char *filename = git_path("FETCH_HEAD");
+               FILE *fp = fopen(filename, "w");
+               if (!fp)
+                       return error("cannot open %s: %s\n", filename,
strerror(errno));
+               fclose(fp);

+       }

        ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);

--
1.5.3.6.861.gd794-dirty



-- 
[]s,
André Goddard

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch --]
[-- Type: text/x-patch; name=0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch, Size: 3642 bytes --]

From dbadc5213b9957fb575c6da8528e5dd7a3f1f43e Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Andr=C3=A9=20Goddard=20Rosa?= <andre.goddard@gmail.com>
Date: Thu, 22 Nov 2007 20:22:23 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
 permission to the repository.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 builtin-fetch--tool.c |   12 ++++++++++--
 builtin-fetch.c       |   21 ++++++++++++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 	if (!strcmp("append-fetch-head", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;
 
 		if (argc != 8)
 			return error("append-fetch-head takes 6 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = append_fetch_head(fp, argv[2], argv[3],
 					   argv[4], argv[5],
 					   argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 	if (!strcmp("native-store", argv[1])) {
 		int result;
 		FILE *fp;
+		char *filename;
 
 		if (argc != 5)
 			return error("fetch-native-store takes 3 args");
-		fp = fopen(git_path("FETCH_HEAD"), "a");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "a");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
 		result = fetch_native_store(fp, argv[2], argv[3], argv[4],
 					    verbose, force);
 		fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..84c8ed4 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -255,7 +255,7 @@ static int update_local_ref(struct ref *ref,
 	}
 }
 
-static void store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, struct ref *ref_map)
 {
 	FILE *fp;
 	struct commit *commit;
@@ -263,8 +263,13 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
+	char *filename = git_path("FETCH_HEAD");
 
-	fp = fopen(git_path("FETCH_HEAD"), "a");
+	fp = fopen(filename, "a");
+	if (!fp) {
+		error("cannot open %s: %s\n", filename, strerror(errno));
+		return 1;
+	}
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;
 
@@ -335,6 +340,7 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
 		}
 	}
 	fclose(fp);
+	return 0;
 }
 
 /*
@@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	if (ret)
 		ret = transport_fetch_refs(transport, ref_map);
 	if (!ret)
-		store_updated_refs(transport->url, ref_map);
+		ret |= store_updated_refs(transport->url, ref_map);
 	transport_unlock_pack(transport);
 	return ret;
 }
@@ -487,8 +493,13 @@ static int do_fetch(struct transport *transport,
 		die("Don't know how to fetch from %s", transport->url);
 
 	/* if not appending, truncate FETCH_HEAD */
-	if (!append)
-		fclose(fopen(git_path("FETCH_HEAD"), "w"));
+	if (!append) {
+		char *filename = git_path("FETCH_HEAD");
+		FILE *fp = fopen(filename, "w");
+		if (!fp)
+			return error("cannot open %s: %s\n", filename, strerror(errno));
+		fclose(fp);
+	}
 
 	ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
 
-- 
1.5.3.6.861.gd794-dirty

Re:* [Resend PATCH] Fix segmentation fault when user doesn't have access permission to the repository.

[Junio C Hamano]

"André Goddard Rosa" <andre.goddard@gmail.com> writes:

> On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> ...
> I tested it here before posting but luckly (or not, as I didn't catch
> this when compiling) it worked,
> as a pointer have the sizeof(int) in my x86 platform. >:|

Most of the changes look trivially correct.  Thanks.

> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index be9e3ea..84c8ed4 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> @@ -263,8 +263,13 @@ static void store_updated_refs(const char *url,
> struct ref *ref_map)
>         char note[1024];
>         const char *what, *kind;
>         struct ref *rm;
> +       char *filename = git_path("FETCH_HEAD");
>
> -       fp = fopen(git_path("FETCH_HEAD"), "a");
> +       fp = fopen(filename, "a");
> +       if (!fp) {
> +               error("cannot open %s: %s\n", filename, strerror(errno));
> +               return 1;
> +       }
>         for (rm = ref_map; rm; rm = rm->next) {
>                 struct ref *ref = NULL;

I started to wonder if there was a particular reason you chose to return
1, not -1 (or just say 'return error("cannot open...", ...)')?

> @@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport,
> struct ref *ref_map)
>         if (ret)
>                 ret = transport_fetch_refs(transport, ref_map);
>         if (!ret)
> -               store_updated_refs(transport->url, ref_map);
> +               ret |= store_updated_refs(transport->url, ref_map);
>         transport_unlock_pack(transport);
>         return ret;
>  }

I think the callers of fetch_refs() are interested in seeing only 0 or
non-zero, so it seems more consistent to signal error with negative
return.  I modified the hunk that starts at line 263 to return the
return value from error() and applied.

That made me follow the transport code, fetch_refs_via_pack().  This is
not your code, so Shawn and Daniel are CC'ed.

The code calls fetch_pack() to get the list of refs it fetched, and
discards refs and always returns 0 to signal success.

But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
returns NULL if error is detected (shallow-support side seems to choose
to die but I suspect that is easily fixable to error out as well).

Shouldn't fetch_refs_via_pack() propagate that error to the caller?

---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index 50db980..048df1f 100644
--- a/transport.c
+++ b/transport.c
@@ -655,7 +655,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	free(heads);
 	free_refs(refs);
 	free(dest);
-	return 0;
+	return (refs ? 0 : -1);
 }
 
 static int git_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)

fetch_refs_via_pack() discards status?

[Junio C Hamano]

The code calls fetch_pack() to get the list of refs it fetched, and
discards refs and always returns 0 to signal success.

But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
returns NULL if error is detected (shallow-support side seems to choose
to die but I suspect that is easily fixable to error out as well).

Shouldn't fetch_refs_via_pack() propagate that error to the caller?

---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index 50db980..048df1f 100644
--- a/transport.c
+++ b/transport.c
@@ -655,7 +655,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	free(heads);
 	free_refs(refs);
 	free(dest);
-	return 0;
+	return (refs ? 0 : -1);
 }
 
 static int git_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)

Re: fetch_refs_via_pack() discards status?

[Daniel Barkalow]

On Tue, 4 Dec 2007, Junio C Hamano wrote:

> The code calls fetch_pack() to get the list of refs it fetched, and
> discards refs and always returns 0 to signal success.
> 
> But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
> returns NULL if error is detected (shallow-support side seems to choose
> to die but I suspect that is easily fixable to error out as well).
> 
> Shouldn't fetch_refs_via_pack() propagate that error to the caller?

I think that's right. I think I got as far as having the error status from 
fetch_pack() actually returned correctly, and then failed to look at it. 
I'd personally avoid testing a pointer to freed memory, but that's 
obviously not actually wrong.

	-Daniel
*This .sig left intentionally blank*

Re: fetch_refs_via_pack() discards status?

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 4 Dec 2007, Junio C Hamano wrote:
>
>> The code calls fetch_pack() to get the list of refs it fetched, and
>> discards refs and always returns 0 to signal success.
>> 
>> But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
>> returns NULL if error is detected (shallow-support side seems to choose
>> to die but I suspect that is easily fixable to error out as well).
>> 
>> Shouldn't fetch_refs_via_pack() propagate that error to the caller?
>
> I think that's right. I think I got as far as having the error status from 
> fetch_pack() actually returned correctly, and then failed to look at it. 
> I'd personally avoid testing a pointer to freed memory, but that's 
> obviously not actually wrong.
>
> 	-Daniel

Hmph, is that an Ack that the patchlet is actually a bugfix?

Re: fetch_refs_via_pack() discards status?

[André Goddard Rosa]

On Dec 5, 2007 7:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Daniel Barkalow <barkalow@iabervon.org> writes:
>
> > On Tue, 4 Dec 2007, Junio C Hamano wrote:
> >
> >> The code calls fetch_pack() to get the list of refs it fetched, and
> >> discards refs and always returns 0 to signal success.
> >>
> >> But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
> >> returns NULL if error is detected (shallow-support side seems to choose
> >> to die but I suspect that is easily fixable to error out as well).
> >>
> >> Shouldn't fetch_refs_via_pack() propagate that error to the caller?
> >
> > I think that's right. I think I got as far as having the error status from
> > fetch_pack() actually returned correctly, and then failed to look at it.
> > I'd personally avoid testing a pointer to freed memory, but that's
> > obviously not actually wrong.
> >
> >       -Daniel
>
> Hmph, is that an Ack that the patchlet is actually a bugfix?
>

Hi, Mr. Junio!

     My 2 cents: I think he means that we should not test a freed pointer:

       free_refs(refs);
       free(dest);                     <===
-       return 0;
+       return (refs ? 0 : -1);     <===

Best regards,
-- 
[]s,
André Goddard

Re: fetch_refs_via_pack() discards status?

[Daniel Barkalow]

On Wed, 5 Dec 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Tue, 4 Dec 2007, Junio C Hamano wrote:
> >
> >> The code calls fetch_pack() to get the list of refs it fetched, and
> >> discards refs and always returns 0 to signal success.
> >> 
> >> But builtin-fetch-pack.c::fetch_pack() has error cases.  The function
> >> returns NULL if error is detected (shallow-support side seems to choose
> >> to die but I suspect that is easily fixable to error out as well).
> >> 
> >> Shouldn't fetch_refs_via_pack() propagate that error to the caller?
> >
> > I think that's right. I think I got as far as having the error status from 
> > fetch_pack() actually returned correctly, and then failed to look at it. 
> > I'd personally avoid testing a pointer to freed memory, but that's 
> > obviously not actually wrong.
> >
> > 	-Daniel
> 
> Hmph, is that an Ack that the patchlet is actually a bugfix?

Yes.

Acked-By: Daniel Barkalow <barkalow@iabervon.org>

	-Daniel
*This .sig left intentionally blank*