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

Reply via email to