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

From: Glen Choo <chooglen@google.com>
Date: Jul 26, 2022, 19:42 UTC
Message-ID: <kl6lilnjslsb.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <47c0803c-bd98-0460-1e9f-c37dc3deeb8d@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 24 quoted lines

> On 7/26/2022 1:43 PM, Taylor Blau wrote:
>> On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:
>>>> 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.
>> 
>> That may be too low-level of a place to put this check, but I don't have
>> a strong opinion about it either way (including whether we should have
>> such a BUG() *anywhere* in this series, including
>> git_configset_add_file()).
>
> Since git_configset_add_file() returns an 'int', could we return -1
> if the supplied 'filename' was null? (The correct place to check would
> be down in git_config_from_file_with_options().)
>
> It would save all these checks here.

Hm, IIUC you are suggesting that git_configset_add_file() returns -1 instead of BUG()-ing?

BUG() sounds better IMO, since there really is nothing useful that git_configset_add_file() (and later functions) can do with a NULL file name. Plus, git_configset_add_file() has already reserved -1 to mean "a file was specified but could not be read".

>
> (Also: do we care that we are ignoring the return values in
> read_protected_config()?

I don't think we care (unless this is a style issue). "git config" succeeds even if it encounters non-repo files that can't be read. In a similar vein, I don't think it matters for protected config if we can't read one of the files (e.g. xdg_config) or even all of the files; all that matters is that we've read everything that we can.

>
> Thanks,
> -Stolee