threads / patch / 10967

patchFix segmentation fault when user doesn't have access permission to the repository.

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

## tl;dr

10 messages between Nov 22, 2007 and Dec 6, 2007. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

André Goddard Rosa· Nov 22, 2007, 00:59 UTC · lore
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(-)
Show changes to 2 files +21 −5

builtin-fetch--tool.c, builtin-fetch.c

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


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
Alex Riesen· Nov 22, 2007, 16:09 UTC · re: André Goddard Rosa · lore

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

André Goddard Rosa, Thu, Nov 22, 2007 01:59:00 +0100:
Show 9 quoted lines
> @@ -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".
Show 10 quoted lines
> +		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
>
André Goddard Rosa· Nov 22, 2007, 22:27 UTC · re: Alex Riesen · lore

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

On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 13 quoted lines
> 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(-)
Show changes to 2 files +26 −7

builtin-fetch--tool.c, builtin-fetch.c

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


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
André Goddard Rosa· Nov 25, 2007, 21:39 UTC · re: André Goddard Rosa · lore

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

On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 13 quoted lines
> 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(-)
Show changes to 2 files +26 −8

builtin-fetch--tool.c, builtin-fetch.c

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


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
Junio C Hamano· Nov 30, 2007, 21:22 UTC · re: André Goddard Rosa · lore

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

"André Goddard Rosa" <andre.goddard@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 19 quoted lines
> 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...", ...)')?

Show 10 quoted lines
> @@ -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(-)
Show changes to transport.c +1 −1
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)
Junio C Hamano· Dec 5, 2007, 07:01 UTC · re: Junio C Hamano · lore

fetch_refs_via_pack() discards status?

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(-)
Show changes to transport.c +1 −1
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)
Daniel Barkalow· Dec 5, 2007, 19:16 UTC · re: Junio C Hamano · lore

Re: fetch_refs_via_pack() discards status?

On Tue, 4 Dec 2007, Junio C Hamano wrote:
Show 8 quoted lines
> 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*
Junio C Hamano· Dec 5, 2007, 21:06 UTC · re: Daniel Barkalow · lore

Re: fetch_refs_via_pack() discards status?

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 17 quoted lines
> 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?
André Goddard Rosa· Dec 6, 2007, 00:38 UTC · re: Junio C Hamano · lore

Re: fetch_refs_via_pack() discards status?

On Dec 5, 2007 7:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> 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
Daniel Barkalow· Dec 6, 2007, 14:09 UTC · re: Junio C Hamano · lore

Re: fetch_refs_via_pack() discards status?

On Wed, 5 Dec 2007, Junio C Hamano wrote:
Show 21 quoted lines
> 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*

← back to recent threads