Re:* [Resend PATCH] Fix segmentation fault when user doesn't have access permission to the repository.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 30, 2007, 21:22 UTC
- Message-ID
- <7v3aunqvha.fsf_-_@gitster.siamese.dyndns.org>
- In-Reply-To
- <b8bf37780711251339y796286fbj2cd8d9225008e13@mail.gmail.com>
"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(-)
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)