[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re:* [Resend PATCH] Fix segmentation fault when user doesn't have access permission to the repositor
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2007-11-30 21:22:41
Message-ID: 7v3aunqvha.fsf_-_ () gitster ! siamese ! dyndns ! org
[Download RAW message or body]

"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)
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]