Matthieu Moy <Matthieu.Moy@xxxxxxx> 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@xxxxxxx>
> ---
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html