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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 12, 2012, 19:34 UTC
Message-ID: <vpq1ukgai4e.fsf@bauges.imag.fr>
In-Reply-To: <7vtxxcc36v.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 35 quoted lines

> 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?

I don't think so. If user_config is NULL, it means something went wrong, because $HOME is unset. In this case, I'd rather die than using some other configuration file silently (which would be possible if $XDG_CONFIG_HOME is set), and this is what the code does:

		if (user_config && access(user_config, R_OK) &&
		    xdg_config && !access(xdg_config, R_OK))
			given_config_file = xdg_config;
		else if (user_config)
			given_config_file = user_config;
		else
			die("$HOME not set");

Perhaps it could actually be made even clearer with

		if (!user_config)
			die("$HOME not set");
		else if (access(user_config, R_OK) &&
			 xdg_config && !access(xdg_config, R_OK))
			given_config_file = xdg_config;
		else
			given_config_file = user_config;

That said, I don't care very strongly about it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/