Re: [PATCH 3/5] config: add BUG() statement instead of possible segfault

[Derrick Stolee <derrickstolee@github.com>]

On 9/27/2022 12:17 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> The git_die_config() method calls git_config_get_value_multi() but
>> immediately navigates to its first value without checking if the result
>> is NULL or empty. Callers should only call git_die_config() if there is
>> at least one value for the given 'key', but such a mistaken use might
>> slip through. It would be better to show a BUG() statement than a
>> possible segfault.
 
> AFAIKT the intent of the current code on "master" is that this will only
> get called if the likes of git_configset_get_string() returns < 0, not
> if it returns > 0.
> 
> So isn't the combination of your 1/5 and this 3/5 now conflating these
> two conditions? See e.g. repo_config_get_string_tmp() and when it would
> call git_die_config().
> 
> I.e. isn't the whole point of git_die_config() to print an error message
> about a configuration *value* that we've parsed out of the config?
> 
> If e.g. the key itself is bad we'll get a -1, but in this case it seems
> we would have a BUG(), but it's not that we "expected a non-empty list
> of values", but that the state of the world changed between our previous
> configset invocation, no?

If git_die_config() was static to config.c, then I would agree with you
that its use is controlled enough to avoid that possibility. However, it
is available in config.h and its doc comment does not say anything about
"make sure 'key' properly parses at least one value".

The point is defense-in-depth to prevent a segfault if someone adds an
incorrect caller. It looks too easy to add an erroneous caller.

Thanks,
-Stolee