Re: [PATCH 2/2] daemon: report permission denied error to clients

From: Jeff King <peff@peff.net>
Date: Oct 17, 2011, 02:09 UTC
Message-ID: <20111017020912.GB18536@sigill.intra.peff.net>
In-Reply-To: <1318803076-4229-2-git-send-email-drizzd@aon.at>

On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:

Show 13 quoted lines

> If passed an inaccessible url, git daemon returns the
> following error:
> 
>  $ git clone git://host/repo
>  fatal: remote error: no such repository: /repo
> 
> In case of a permission denied error, return the following
> instead:
> 
>  fatal: remote error: permission denied: /repo
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---

I like the intent. This actually does leak a little more information than the existing --informative-errors, as before you couldn't tell the difference between "not found" and "not exported". But I think the spirit of --informative-errors is to let that information leak, and this is a good change.

Show 18 quoted lines

> -static char *path_ok(char *directory)
> +static int path_ok(char *directory, const char **return_path)
>  {
>  	static char rpath[PATH_MAX];
>  	static char interp_path[PATH_MAX];
> @@ -120,13 +120,13 @@ static char *path_ok(char *directory)
>  
>  	if (daemon_avoid_alias(dir)) {
>  		logerror("'%s': aliased", dir);
> -		return NULL;
> +		return -1;
>  	}
>  
>  	if (*dir == '~') {
>  		if (!user_path) {
>  			logerror("'%s': User-path not allowed", dir);
> -			return NULL;
> +			return EACCES;

The new calling conventions for this function seem a little weird. I would expect either "return negative, and set errno" for usual library code, or possibly "return negative error value". But "return -1, or a positive error code" seems unusual to me.

One of:

  errno = EACCESS;
  return -1;

or

  return -EACCESS;

would be more idiomatic, I think.

-Peff