diff mbox series

[09/10] config API: add "string" version of *_value_multi(), fix segfaults

Message ID patch-09.10-bda9d504b89-20221026T151328Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series config API: make "multi" safe, fix numerous segfaults | expand

Commit Message

Ævar Arnfjörð Bjarmason Oct. 26, 2022, 3:35 p.m. UTC 
Fix numerous and mostly long-standing segfaults in consumers of
the *_config_*value_multi() API. As discussed in the preceding commit
an empty key in the config syntax yields a "NULL" string, which these
users would give to strcmp() (or similar), resulting in segfaults.

As this change shows, these non-test users of
the *_config_*value_multi() API didn't really want such an an unsafe
and low-level API, let's give them something with the safety of
git_config_get_string() instead.

This fix is similar to what the *_string() functions and others
acquired in[1] and [2]. Namely introducing and using a
safer *_config_*value_multi_string() variant of the
low-level *_config_*value_multi_string() function.

This fixes segfaults in code introduced in:

  - d811c8e17c6 (versionsort: support reorder prerelease suffixes, 2015-02-26)
  - c026557a373 (versioncmp: generalize version sort suffix reordering, 2016-12-08)
  - a086f921a72 (submodule: decouple url and submodule interest, 2017-03-17)
  - a6be5e6764a (log: add log.excludeDecoration config option, 2020-04-16)
  - 92156291ca8 (log: add default decoration filter, 2022-08-05)
  - 50a044f1e40 (gc: replace config subprocesses with API calls, 2022-09-27)

There are now two remaining user of the low-level API, one is the
"t/helper/test-config.c" code added in [3]. The other we'll address in
a subsequent commit.

As seen in the preceding commit we need to give the
"t/helper/test-config.c" caller these "NULL" entries. We thus cannot
alter the underlying git_configset_get_value_multi_1() function itself
to make it "safe".

Such a thing would also be undesirable, as casting or forbidding NULL
values might only be one potential use-case of the underlying
function. It's better to have a "raw" low-level function, and
corresponding wrapper functions that coerce its values. The callback
pattern being used here will make it easy to introduce e.g. a "multi"
variant which coerces its values to "bool", "int", "path" etc.

1. 40ea4ed9032 (Add config_error_nonbool() helper function,
   2008-02-11)
2. 6c47d0e8f39 (config.c: guard config parser from value=NULL,
   2008-02-11).
3. 4c715ebb96a (test-config: add tests for the config_set API,
   2014-07-28)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c                   |  4 +--
 builtin/log.c                  |  2 +-
 config.c                       | 63 +++++++++++++++++++++++++++++++---
 config.h                       | 40 +++++++++++++++++++++
 pack-bitmap.c                  |  2 +-
 submodule.c                    |  2 +-
 t/t4202-log.sh                 | 15 ++++++++
 t/t5310-pack-bitmaps.sh        | 21 ++++++++++++
 t/t7004-tag.sh                 | 17 +++++++++
 t/t7413-submodule-is-active.sh | 16 +++++++++
 t/t7900-maintenance.sh         | 38 ++++++++++++++++++++
 versioncmp.c                   |  8 ++---
 12 files changed, 214 insertions(+), 14 deletions(-)

Comments

Junio C Hamano Oct. 27, 2022, 7:49 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix numerous and mostly long-standing segfaults in consumers of
> the *_config_*value_multi() API. As discussed in the preceding commit
> an empty key in the config syntax yields a "NULL" string, which these
> users would give to strcmp() (or similar), resulting in segfaults.

Sounds like a good idea.

I would have called them _nonbool(), not _string(), especially
because we are not going to have other variants like _int(), though.
Junio C Hamano Oct. 27, 2022, 7:52 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Fix numerous and mostly long-standing segfaults in consumers of
>> the *_config_*value_multi() API. As discussed in the preceding commit
>> an empty key in the config syntax yields a "NULL" string, which these
>> users would give to strcmp() (or similar), resulting in segfaults.
>
> Sounds like a good idea.
>
> I would have called them _nonbool(), not _string(), especially
> because we are not going to have other variants like _int(), though.

Actually, I take it back.  Instead of introducing _string(), how
about introducing _bool() and convert those minority callers that do
want to see boolean values to use the new one, while rejecting NULLs
for everybody else that calls the traditional "get_value" family of
functions?  That would "optimize" for the majority of simpler users,
wouldn't it?
Ævar Arnfjörð Bjarmason Oct. 27, 2022, 11:44 p.m. UTC | #3 
On Thu, Oct 27 2022, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> Fix numerous and mostly long-standing segfaults in consumers of
>>> the *_config_*value_multi() API. As discussed in the preceding commit
>>> an empty key in the config syntax yields a "NULL" string, which these
>>> users would give to strcmp() (or similar), resulting in segfaults.
>>
>> Sounds like a good idea.
>>
>> I would have called them _nonbool(), not _string(), especially
>> because we are not going to have other variants like _int(), though.
>
> Actually, I take it back.  Instead of introducing _string(), how
> about introducing _bool() and convert those minority callers that do
> want to see boolean values to use the new one, while rejecting NULLs
> for everybody else that calls the traditional "get_value" family of
> functions?  That would "optimize" for the majority of simpler users,
> wouldn't it?

I don't think the goal should be just to optimize for those current
users, but to leave the config API in a state where it makes sense
conceptually.

For the scalar (single) values we have a low-level "low-level" function,
and then variants to get it as a bool, path, string, int etc.

I think a "multi" function should just be the logical result of applying
one of those "types" to list. I.e. (pseudocode):

	a_raw = get_config_raw("a.key");
	a_string = stringify(a_raw);

And, as a list:

	list_raw = get_config_raw_multi("a.key");
	list_strings = map { stringify(item) } list_raw;

Now, if we don't supply the equivalent of the "raw, but multi-value"
function we'll make it hard to use the API, because now you can't think
about it as the "multi" just being a list version of what you get with
the scalar version.

E.g. what should we do about "[a]key" in a list API that stringifies by
default? If you then want "stringified bool" we're only left with bad choices:

 - If you die that's bed, because that's a legit true value
 - If you coerce it to "" to help the string use case you get the wrong
   answer, because "[a]key=" (empty string) is false, but "[a]key"
   (value-less) is true.
 - Ditto if you prune it out, as then it won't be seen in the bool list.

Which is why I went for the end-state here. I.e. it's now easy to add
other "multi" variants (we'd need to add coercion, but that's easy
enough).
Junio C Hamano Oct. 28, 2022, 7:16 p.m. UTC | #4 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Actually, I take it back.  Instead of introducing _string(), how
>> about introducing _bool() and convert those minority callers that do
>> want to see boolean values to use the new one, while rejecting NULLs
>> for everybody else that calls the traditional "get_value" family of
>> functions?  That would "optimize" for the majority of simpler users,
>> wouldn't it?
>
> I don't think the goal should be just to optimize for those current
> users, but to leave the config API in a state where it makes sense
> conceptually.

It is more like guiding a conceptually clean design using the need
of the current users to rein in pursuit of theoretical "elegance".

> Now, if we don't supply the equivalent of the "raw, but multi-value"
> function we'll make it hard to use the API, because now you can't think
> about it as the "multi" just being a list version of what you get with
> the scalar version.

I am not interested in _bool() variant that "stringifies" NULL to
"true" at all.  What I was suggesting was:

 * Reserve the current get and get_multi for those who should have
   been calling config_error_nonbool() themselves (because your
   _string() has not been available to them, they were lazy not to
   bother, leading to NULL dereference given certain end-user data).
   And do the config_error_nonbool() inside the updated get and
   get_multi without introducing _string() variant at all.

 * The above alone WILL break callers who are prepared to handle
   "bool" and "bool plus some other string", because they are fully
   expecting that the get API will give them NULL but the above
   update will instead stop before they see the NULL they are
   prepared to handle themselves.  Introduce _bool variants and make
   them call them.

without any "stringifying" at all.
Ævar Arnfjörð Bjarmason Oct. 31, 2022, 6:22 p.m. UTC | #5 
On Fri, Oct 28 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Actually, I take it back.  Instead of introducing _string(), how
>>> about introducing _bool() and convert those minority callers that do
>>> want to see boolean values to use the new one, while rejecting NULLs
>>> for everybody else that calls the traditional "get_value" family of
>>> functions?  That would "optimize" for the majority of simpler users,
>>> wouldn't it?
>>
>> I don't think the goal should be just to optimize for those current
>> users, but to leave the config API in a state where it makes sense
>> conceptually.
>
> It is more like guiding a conceptually clean design using the need
> of the current users to rein in pursuit of theoretical "elegance".

I agree that the current code users don't care either way, they'll be
getting the same thing.

But being able to readily understand an API is valuable too. The config
API is bad enough with all repetition of:

	{git_configset,repo_config,git_config}_get_value()
	{git_configset,repo_config,git_config}_get_string()
	{git_configset,repo_config,git_config}_get_bool()
        [...]

I think it's worth it to be able to say that:

	{git_configset,repo_config,git_config}_get_value_multi()
	{git_configset,repo_config,git_config}_get_value_string()
        <ditto "bool">

Are "just like the scalar version, but multi". Actually when I summarize
it like that I realize I should really make it:

	{git_configset,repo_config,git_config}_get_string_multi()

I.e. "*_get_string_multi()", not "*_get_value_multi_string()". I don't
know what I was thinking.

But aside from that, the point is I think it's worth it not to have it
instead be:

        # "non-string" doesn't exist, but get it via some use of
        # (currently static) configset_find_element()

        # "get value", but really "get string, for multi"
	{git_configset,repo_config,git_config}_get_value_multi()

        # ???
	{git_configset,repo_config,git_config}_get_bool_multi()

We currently don't need/have a "*_get_bool_multi()", which I think is
besides the point. We might in the future, and should forsee that we're
picking a nonsensical naming convention.

We also have similar gaps in the current API (not all variants of all
functions exist, for the scalar variants), but at least those that we do
have behave consistently.

>> Now, if we don't supply the equivalent of the "raw, but multi-value"
>> function we'll make it hard to use the API, because now you can't think
>> about it as the "multi" just being a list version of what you get with
>> the scalar version.
>
> I am not interested in _bool() variant that "stringifies" NULL to
> "true" at all.  What I was suggesting was:
>
>  * Reserve the current get and get_multi for those who should have
>    been calling config_error_nonbool() themselves (because your
>    _string() has not been available to them, they were lazy not to
>    bother, leading to NULL dereference given certain end-user data).
>    And do the config_error_nonbool() inside the updated get and
>    get_multi without introducing _string() variant at all.

I get what you're saying, I just think it suffers from the problem
outlined above, and that it's worth solving it.

>  * The above alone WILL break callers who are prepared to handle
>    "bool" and "bool plus some other string", because they are fully
>    expecting that the get API will give them NULL but the above
>    update will instead stop before they see the NULL they are
>    prepared to handle themselves.  Introduce _bool variants and make
>    them call them.

Even if it wasn't for the naming question, I think the arrangement in
this series is also better in that I need to go and change each caller
to the new variant, and explain for each one why it's OK.

Whereas if we just sneakconfig_error_nonbool() into the low-level API
we're going to have a smaller change, but also one that's basically
"trust me, I read the code of all the callers, this should be fine...".
diff mbox series

Patch

diff --git a/builtin/gc.c b/builtin/gc.c
index 3e94fa5e20f..3fc759b1f0c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1481,7 +1481,7 @@  static int maintenance_register(int argc, const char **argv, const char *prefix)
 	if (git_config_lookup_value("maintenance.strategy"))
 		git_config_set("maintenance.strategy", "incremental");
 
-	if (!git_config_get_knownkey_value_multi(key, &list))
+	if (!git_config_get_knownkey_value_multi_string(key, &list))
 		found = unsorted_string_list_has_string(list, maintpath);
 
 	if (!found) {
@@ -1530,7 +1530,7 @@  static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		usage_with_options(builtin_maintenance_unregister_usage,
 				   options);
 
-	if (!git_config_get_knownkey_value_multi(key, &list))
+	if (!git_config_get_knownkey_value_multi_string(key, &list))
 		found = unsorted_string_list_has_string(list, maintpath);
 
 	if (found) {
diff --git a/builtin/log.c b/builtin/log.c
index 75464c96ccf..d6b1c75ea2e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -184,7 +184,7 @@  static void set_default_decoration_filter(struct decoration_filter *decoration_f
 	struct string_list *include = decoration_filter->include_ref_pattern;
 	const struct string_list *config_exclude;
 
-	if (!git_config_get_knownkey_value_multi("log.excludeDecoration",
+	if (!git_config_get_knownkey_value_multi_string("log.excludeDecoration",
 					      &config_exclude)) {
 		struct string_list_item *item;
 		for_each_string_list_item(item, config_exclude)
diff --git a/config.c b/config.c
index 5cd130ddbb9..25bb6514f81 100644
--- a/config.c
+++ b/config.c
@@ -2428,7 +2428,8 @@  int git_configset_get_value(struct config_set *cs, const char *key, const char *
 
 static int git_configset_get_value_multi_1(struct config_set *cs, const char *key,
 					   const struct string_list **dest,
-					   int read_only, int knownkey)
+					   int read_only, int knownkey,
+					   string_list_each_func_t check_fn)
 {
 	struct config_set_element *e;
 	int ret;
@@ -2440,28 +2441,51 @@  static int git_configset_get_value_multi_1(struct config_set *cs, const char *ke
 		return ret;
 	else if (!e)
 		return 1;
+	if (check_fn &&
+	    (ret = for_each_string_list(&e->value_list, check_fn, (void *)key)))
+		return ret;
 	if (!read_only)
 		*dest = &e->value_list;
 
 	return 0;
 }
 
+static int check_multi_string(struct string_list_item *item, void *util)
+{
+	return item->string ? 0 : config_error_nonbool(util);
+}
+
 int git_configset_get_value_multi(struct config_set *cs, const char *key,
 				  const struct string_list **dest)
 {
-	return git_configset_get_value_multi_1(cs, key, dest, 0, 0);
+	return git_configset_get_value_multi_1(cs, key, dest, 0, 0, NULL);
 }
 
 int git_configset_get_knownkey_value_multi(struct config_set *cs,
 					   const char *const key,
 					   const struct string_list **dest)
 {
-	return git_configset_get_value_multi_1(cs, key, dest, 0, 1);
+	return git_configset_get_value_multi_1(cs, key, dest, 0, 1, NULL);
+}
+
+int git_configset_get_value_multi_string(struct config_set *cs, const char *key,
+					 const struct string_list **dest)
+{
+	return git_configset_get_value_multi_1(cs, key, dest, 0, 0,
+					       check_multi_string);
+}
+
+int git_configset_get_knownkey_value_multi_string(struct config_set *cs,
+						  const char *const key,
+						  const struct string_list **dest)
+{
+	return git_configset_get_value_multi_1(cs, key, dest, 0, 1,
+					       check_multi_string);
 }
 
 int git_configset_lookup_value(struct config_set *cs, const char *key)
 {
-	return git_configset_get_value_multi_1(cs, key, NULL, 1, 0);
+	return git_configset_get_value_multi_1(cs, key, NULL, 1, 0, NULL);
 }
 
 int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
@@ -2603,7 +2627,8 @@  void repo_config(struct repository *repo, config_fn_t fn, void *data)
 int repo_config_lookup_value(struct repository *repo, const char *key)
 {
 	git_config_check_init(repo);
-	return git_configset_get_value_multi_1(repo->config, key, NULL, 1, 0);
+	return git_configset_get_value_multi_1(repo->config, key, NULL, 1, 0,
+					       NULL);
 }
 
 int repo_config_get_value(struct repository *repo,
@@ -2629,6 +2654,22 @@  int repo_config_get_knownkey_value_multi(struct repository *repo,
 	return git_configset_get_knownkey_value_multi(repo->config, key, dest);
 }
 
+int repo_config_get_value_multi_string(struct repository *repo,
+				       const char *key,
+				       const struct string_list **dest)
+{
+	git_config_check_init(repo);
+	return git_configset_get_value_multi_string(repo->config, key, dest);
+}
+
+int repo_config_get_knownkey_value_multi_string(struct repository *repo,
+						const char *key,
+						const struct string_list **dest)
+{
+	git_config_check_init(repo);
+	return git_configset_get_knownkey_value_multi_string(repo->config, key, dest);
+}
+
 int repo_config_get_string(struct repository *repo,
 			   const char *key, char **dest)
 {
@@ -2759,6 +2800,18 @@  int git_config_get_knownkey_value_multi(const char *const key,
 	return repo_config_get_knownkey_value_multi(the_repository, key, dest);
 }
 
+int git_config_get_value_multi_string(const char *key,
+				      const struct string_list **dest)
+{
+	return repo_config_get_value_multi_string(the_repository, key, dest);
+}
+
+int git_config_get_knownkey_value_multi_string(const char *key,
+					       const struct string_list **dest)
+{
+	return repo_config_get_knownkey_value_multi_string(the_repository, key, dest);
+}
+
 int git_config_get_string(const char *key, char **dest)
 {
 	return repo_config_get_string(the_repository, key, dest);
diff --git a/config.h b/config.h
index cf1ae7862a8..047ef83afc6 100644
--- a/config.h
+++ b/config.h
@@ -484,6 +484,30 @@  int git_configset_get_knownkey_value_multi(struct config_set *cs,
 					   const char *const key,
 					   const struct string_list **dest);
 
+/**
+ * A validation wrapper for git_configset_get_value_multi() which does
+ * for it what git_configset_get_string() does for
+ * git_configset_get_value().
+ *
+ * The configuration syntax allows for "[section] key", which will
+ * give us a NULL entry in the "struct string_list", as opposed to
+ * "[section] key =" which is the empty string. Most users of the API
+ * are not prepared to handle NULL in a "struct string_list".
+ */
+int git_configset_get_value_multi_string(struct config_set *cs, const char *key,
+					 const struct string_list **dest);
+
+/**
+ * A wrapper for git_configset_get_value_multi_string() which does for
+ * it what git_configset_get_knownkey_value_multi() does for
+ * git_configset_get_value_multi().
+ */
+RESULT_MUST_BE_USED
+int git_configset_get_knownkey_value_multi_string(struct config_set *cs,
+						  const char *const key,
+						  const struct string_list **dest);
+
+
 /**
  * Clears `config_set` structure, removes all saved variable-value pairs.
  */
@@ -527,6 +551,14 @@  int repo_config_get_knownkey_value_multi(struct repository *repo,
 					 const char *const key,
 					 const struct string_list **dest);
 RESULT_MUST_BE_USED
+int repo_config_get_value_multi_string(struct repository *repo,
+				       const char *key,
+				       const struct string_list **dest);
+RESULT_MUST_BE_USED
+int repo_config_get_knownkey_value_multi_string(struct repository *repo,
+						const char *const key,
+						const struct string_list **dest);
+RESULT_MUST_BE_USED
 int repo_config_lookup_value(struct repository *repo, const char *key);
 int repo_config_get_string(struct repository *repo,
 			   const char *key, char **dest);
@@ -592,6 +624,14 @@  RESULT_MUST_BE_USED
 int git_config_get_knownkey_value_multi(const char *const key,
 					const struct string_list **dest);
 
+RESULT_MUST_BE_USED
+int git_config_get_value_multi_string(const char *key,
+				      const struct string_list **dest);
+
+RESULT_MUST_BE_USED
+int git_config_get_knownkey_value_multi_string(const char *const key,
+					       const struct string_list **dest);
+
 /**
  * The same as git_config_value(), except without the extra work to
  * return the value to the user, used to check if a value for a key
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 0b4e73abbfa..9a61d9ff9a8 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2303,7 +2303,7 @@  const struct string_list *bitmap_preferred_tips(struct repository *r)
 {
 	const struct string_list *dest;
 
-	if (!repo_config_get_knownkey_value_multi(r, "pack.preferbitmaptips",
+	if (!repo_config_get_knownkey_value_multi_string(r, "pack.preferbitmaptips",
 					       &dest))
 		return dest;
 	return NULL;
diff --git a/submodule.c b/submodule.c
index e8c4362743d..f84b253154e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -274,7 +274,7 @@  int is_tree_submodule_active(struct repository *repo,
 	free(key);
 
 	/* submodule.active is set */
-	if (!repo_config_get_knownkey_value_multi(repo, "submodule.active", &sl)) {
+	if (!repo_config_get_knownkey_value_multi_string(repo, "submodule.active", &sl)) {
 		struct pathspec ps;
 		struct strvec args = STRVEC_INIT;
 		const struct string_list_item *item;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 2ce2b41174d..ae73aef922f 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -835,6 +835,21 @@  test_expect_success 'log.decorate configuration' '
 
 '
 
+test_expect_success 'parse log.excludeDecoration with no value' '
+	cp .git/config .git/config.orig &&
+	test_when_finished mv .git/config.orig .git/config &&
+
+	cat >>.git/config <<-\EOF &&
+	[log]
+		excludeDecoration
+	EOF
+	cat >expect <<-\EOF &&
+	error: missing value for '\''log.excludeDecoration'\''
+	EOF
+	git log --decorate=short 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'decorate-refs with glob' '
 	cat >expect.decorate <<-\EOF &&
 	Merge-tag-reach
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 6d693eef82f..68195a1de36 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -404,6 +404,27 @@  test_bitmap_cases () {
 		)
 	'
 
+	test_expect_success 'pack.preferBitmapTips' '
+		git init repo &&
+		test_when_finished "rm -rf repo" &&
+		(
+			cd repo &&
+			git config pack.writeBitmapLookupTable '"$writeLookupTable"' &&
+			test_commit_bulk --message="%s" 103 &&
+
+			cat >>.git/config <<-\EOF &&
+			[pack]
+				preferBitmapTips
+			EOF
+
+			cat >expect <<-\EOF &&
+			error: missing value for '\''pack.preferbitmaptips'\''
+			EOF
+			git repack -adb 2>actual &&
+			test_cmp expect actual
+		)
+	'
+
 	test_expect_success 'complains about multiple pack bitmaps' '
 		rm -fr repo &&
 		git init repo &&
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 9aa1660651b..f4a31ada79a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1843,6 +1843,23 @@  test_expect_success 'invalid sort parameter in configuratoin' '
 	test_must_fail git tag -l "foo*"
 '
 
+test_expect_success 'version sort handles empty value for versionsort.{prereleaseSuffix,suffix}' '
+	cp .git/config .git/config.orig &&
+	test_when_finished mv .git/config.orig .git/config &&
+
+	cat >>.git/config <<-\EOF &&
+	[versionsort]
+		prereleaseSuffix
+		suffix
+	EOF
+	cat >expect <<-\EOF &&
+	error: missing value for '\''versionsort.suffix'\''
+	error: missing value for '\''versionsort.prereleasesuffix'\''
+	EOF
+	git tag -l --sort=version:refname 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'version sort with prerelease reordering' '
 	test_config versionsort.prereleaseSuffix -rc &&
 	git tag foo1.6-rc1 &&
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
index 7cdc2637649..887d181b72e 100755
--- a/t/t7413-submodule-is-active.sh
+++ b/t/t7413-submodule-is-active.sh
@@ -51,6 +51,22 @@  test_expect_success 'is-active works with submodule.<name>.active config' '
 	test-tool -C super submodule is-active sub1
 '
 
+test_expect_success 'is-active handles submodule.active config missing a value' '
+	cp super/.git/config super/.git/config.orig &&
+	test_when_finished mv super/.git/config.orig super/.git/config &&
+
+	cat >>super/.git/config <<-\EOF &&
+	[submodule]
+		active
+	EOF
+
+	cat >expect <<-\EOF &&
+	error: missing value for '\''submodule.active'\''
+	EOF
+	test-tool -C super submodule is-active sub1 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'is-active works with basic submodule.active config' '
 	test_when_finished "git -C super config submodule.sub1.URL ../sub" &&
 	test_when_finished "git -C super config --unset-all submodule.active" &&
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 96bdd420456..1201866c8d0 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -505,6 +505,44 @@  test_expect_success 'register and unregister' '
 	git maintenance unregister --force
 '
 
+test_expect_success 'register with no value for maintenance.repo' '
+	cp .git/config .git/config.orig &&
+	test_when_finished mv .git/config.orig .git/config &&
+
+	cat >>.git/config <<-\EOF &&
+	[maintenance]
+		repo
+	EOF
+	cat >expect <<-\EOF &&
+	error: missing value for '\''maintenance.repo'\''
+	EOF
+	git maintenance register 2>actual &&
+	test_cmp expect actual &&
+	git config maintenance.repo
+'
+
+test_expect_success 'unregister with no value for maintenance.repo' '
+	cp .git/config .git/config.orig &&
+	test_when_finished mv .git/config.orig .git/config &&
+
+	cat >>.git/config <<-\EOF &&
+	[maintenance]
+		repo
+	EOF
+	cat >expect <<-\EOF &&
+	error: missing value for '\''maintenance.repo'\''
+	EOF
+	test_expect_code 128 git maintenance unregister 2>actual.raw &&
+	grep ^error actual.raw >actual &&
+	test_cmp expect actual &&
+	git config maintenance.repo &&
+
+	git maintenance unregister --force 2>actual.raw &&
+	grep ^error actual.raw >actual &&
+	test_cmp expect actual &&
+	git config maintenance.repo
+'
+
 test_expect_success !MINGW 'register and unregister with regex metacharacters' '
 	META="a+b*c" &&
 	git init "$META" &&
diff --git a/versioncmp.c b/versioncmp.c
index effe1a6a6be..4efb5f9e621 100644
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -165,11 +165,11 @@  int versioncmp(const char *s1, const char *s2)
 
 		initialized = 1;
 		prereleases_ret =
-			git_config_get_knownkey_value_multi("versionsort.suffix",
-							    &prereleases);
+			git_config_get_knownkey_value_multi_string("versionsort.suffix",
+								   &prereleases);
 		deprecated_prereleases_ret =
-			git_config_get_knownkey_value_multi("versionsort.prereleasesuffix",
-							    &deprecated_prereleases);
+			git_config_get_knownkey_value_multi_string("versionsort.prereleasesuffix",
+								   &deprecated_prereleases);
 
 		if (!prereleases_ret) {
 			if (!deprecated_prereleases_ret)