Re: [PATCH] config: fix several access(NULL) calls

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> When $HOME is unset, home_config_paths fails and returns NULL pointers
> for user_config and xdg_config. Valgrind complains with Syscall param
> access(pathname) points to unaddressable byte(s).
>
> Don't call blindly access() on these variables, but test them for
> NULL-ness before.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>> This patch causes valgrind warnings in t1300.81 (get --path copes with
>> unset $HOME) about passing NULL to access():
>
> Indeed. The following patch should fix it.
>
>  builtin/config.c | 3 ++-
>  config.c         | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index e8e1c0a..67945b2 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -387,7 +387,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  
>  		home_config_paths(&user_config, &xdg_config, "config");
>  
> -		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
> +		if (user_config && access(user_config, R_OK) &&
> +		    xdg_config && !access(xdg_config, R_OK))
>  			given_config_file = xdg_config;

Shouldn't we be using xdg_config, if user_config is NULL and
xdg_config is defined and accessible?

In other words, isn't the objective of this "fix" is to replace any
call to access(PATH, PERM) whose PATH can potentially be NULL with

	(PATH ? access(PATH, PERM) : -1)

because we want to pretend access(PATH, PERM) returned an error,
saying "The variable PATH holds a path to the file that is not
accessible", when PATH is NULL?

And that is equivalent to

	(!PATH || access(PATH, PERM))

in the context of boolean.  The original

	if (access(user_config, R_OK) && !access(xdg_config, R_OK))

becomes 

	if ((!user_config || access(user_config, R_OK)) &&
	    !(!xdg_config || access(xdg_config, R_OK)))

and the latter half of it is the same as

	    (xdg_config && !access(xdg_config, R_OK))

but the former half is not. Shouldn't it be

	if ((!user_config || access(user_config, R_OK)) &&
	    (xdg_config && !access(xdg_config, R_OK)))

Confused.

> diff --git a/config.c b/config.c
> index d28a499..6b97503 100644
> --- a/config.c
> +++ b/config.c
> @@ -940,12 +940,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  		found += 1;
>  	}
>  
> -	if (!access(xdg_config, R_OK)) {
> +	if (xdg_config && !access(xdg_config, R_OK)) {
>  		ret += git_config_from_file(fn, xdg_config, data);
>  		found += 1;
>  	}
>  
> -	if (!access(user_config, R_OK)) {
> +	if (user_config && !access(user_config, R_OK)) {
>  		ret += git_config_from_file(fn, user_config, data);
>  		found += 1;
>  	}