On 9/27/2022 12:17 PM, Ævar Arnfjörð Bjarmason wrote:

On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:

quoted

From: Derrick Stolee <redacted>

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