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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 26, 2022, 17:51 UTC
Message-ID: <47c0803c-bd98-0460-1e9f-c37dc3deeb8d@github.com>
In-Reply-To: <YuAn171BhecC1w+O@nand.local>

On 7/26/2022 1:43 PM, Taylor Blau wrote:

Show 17 quoted lines

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

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

Thanks, -Stolee