Re: [PATCH] config.c: NULL check when reading protected config

[Glen Choo <chooglen@google.com>]

Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Jul 26, 2022 at 05:09:32PM +0000, Glen Choo via GitGitGadget wrote:
>> From: Glen Choo <chooglen@google.com>
>>
>> In read_protected_config(), check whether each file name is NULL before
>> attempting to read it. This mirrors do_git_config_sequence() (which
>> read_protected_config() is modelled after).
>
> s/modelled/modeled

Ah, thanks.

>> Without these NULL checks,
>>
>>  make SANITIZE=address test T=t0410*.sh
>
> I'm glad that t0410 was catching this for us already, though it is too
> bad we didn't see it outside of the ASan builds, or I think we could
> have potentially caught this earlier.
>
> Either way, I think the test coverage here is sufficient, so what you
> wrote makes sense.
>
>> diff --git a/config.c b/config.c
>> index 015bec360f5..b0ba7f439a4 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)
>>  	system_config = git_system_config();
>>  	git_global_config(&user_config, &xdg_config);
>>
>> -	git_configset_add_file(&protected_config, system_config);
>> -	git_configset_add_file(&protected_config, xdg_config);
>> -	git_configset_add_file(&protected_config, user_config);
>> +	if (system_config)
>> +		git_configset_add_file(&protected_config, system_config);
>> +	if (xdg_config)
>> +		git_configset_add_file(&protected_config, xdg_config);
>> +	if (user_config)
>> +		git_configset_add_file(&protected_config, user_config);
>>  	git_configset_add_parameters(&protected_config);
>
> I wonder: should it become a BUG() to call git_configset_add_file() with
> a NULL filename? That would have elevated the test failure outside of
> just the ASAn builds, I'd think.
>
> There's certainty a risk of being too defensive, but elevating this
> error beyond just the ASan builds indicates that this would be an
> appropriate layer of defense IMHO.

Hm, if we're going in this direction, what if we made it a BUG() to call
fopen_or_warn() with a NULL filename? Then we wouldn't have to
reimplement this BUG() check in all of its callers.

>
> Thanks,
> Taylor