git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 00/10] config API: make "multi" safe, fix numerous segfaults
@ 2022-10-26 15:35 Ævar Arnfjörð Bjarmason
  2022-10-26 15:35 ` [PATCH 01/10] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
                   ` (11 more replies)
  0 siblings, 12 replies; 134+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-26 15:35 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Jeff King, Taylor Blau,
	SZEDER Gábor, Ævar Arnfjörð Bjarmason

This series is a follow-up to an earlier RFC Stolee sent about making
the *_multi() config API return non-NULL, and instead give you an
empty string list[1].

I also think that part of the config API is a wart, but that we should
go for a different solution. It's the only config function that
doesn't return an "int" indicating whether we found the key.

Code that wants to use the values should then check that return
value. I.e. Stolee's version allows you to do:

	const struct string_list *list = git_config_get_value_multi(key);
	for_each_string_list_item(item, list) { ... found = 1 ... }

Whereas in this proposal we instead do (same as for non-multi):

	if (!git_config_get_const_value_multi(key, &list))
		for_each_string_list_item(item, list) { ... found = 1 ... }

Mid-series that's made nicer by adjusting the string_list API to have
sensible "const"'s (so we don't need catsing), and using utility
functions from there. I.e. the recently added code in builtin/gc.c
becomes (Stolee's at [3]):

	if (!git_config_get_knownkey_value_multi(key, &list))
		found = unsorted_string_list_has_string(list, maintpath);

But anyway.

Once I started poking at this approach I discovered that we have a
much larger issue here than whether the top-level value is NULL or an empty list.

As noted I don't think it's an issue that the *_multi() returns NULL
if we have no key, that's easy to handle.

But *_multi() doesn't have the equivalent of a wrapper that coerces
the values on the list into one of our types (as in "git config
--type=<type>").

A not so well known edge case in our config format (see 8/10) is that
value-less keys are represented as NULL's, and a "struct string_list"
is perfectly happy to have a "char *string" member that's NULL.

I.e.:

	[a]key=x
	[a]key
	[a]key=y

Is represented as:

	{ "x", NULL, "y" }

Not, as existing code apparently expected:

	{ "x", "", "y" }

As a result existing code using *_multi() would segfault when reading
config like that. See 9/10, which fixes segfaults in 6 commits as old
as from 2015, and a couple of recent ones: One in the last release,
and one on "master" but not released yet.

The fix is thoroughly boring, we just start doing for *_multi() what
we've been doing for other config since Junio's 2008 fix (see 9/10) to
fix the same issue for non-multi config variables.

I.e. we provide a safer "I want strings, please" variant of the API,
just for *_multi(). At the culmination of this topic only the
test-helper uses the underlying unsafe API, and only because it needs
to check that we're still parsing the config correctly.

1. https://lore.kernel.org/git/pull.1369.git.1664287711.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/220928.868rm3w9d4.gmgdl@evledraar.gmail.com
3. https://lore.kernel.org/git/e06cb4df081bc2222731f9185a22ed7ad67e3814.1664287711.git.gitgitgadget@gmail.com/

Ævar Arnfjörð Bjarmason (10):
  config API: have *_multi() return an "int" and take a "dest"
  for-each-repo: error on bad --config
  config API: mark *_multi() with RESULT_MUST_BE_USED
  string-list API: mark "struct_string_list" to "for_each_string_list"
    const
  string-list API: make has_string() and list_lookup() "const"
  builtin/gc.c: use "unsorted_string_list_has_string()" where
    appropriate
  config API: add and use "lookup_value" functions
  config tests: add "NULL" tests for *_get_value_multi()
  config API: add "string" version of *_value_multi(), fix segfaults
  for-each-repo: with bad config, don't conflate <path> and <cmd>

 builtin/for-each-repo.c        |  14 ++-
 builtin/gc.c                   |  29 +-----
 builtin/log.c                  |   6 +-
 builtin/submodule--helper.c    |   7 +-
 builtin/worktree.c             |   3 +-
 config.c                       | 164 +++++++++++++++++++++++++++++----
 config.h                       | 110 ++++++++++++++++++++--
 pack-bitmap.c                  |   7 +-
 string-list.c                  |   6 +-
 string-list.h                  |   6 +-
 submodule.c                    |   3 +-
 t/helper/test-config.c         |   6 +-
 t/t0068-for-each-repo.sh       |  19 ++++
 t/t1308-config-set.sh          |  30 ++++++
 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                   |  18 +++-
 20 files changed, 451 insertions(+), 84 deletions(-)

-- 
2.38.0.1251.g3eefdfb5e7a


^ permalink raw reply	[flat|nested] 134+ messages in thread

end of thread, other threads:[~2023-04-07 15:51 UTC | newest]

Thread overview: 134+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-26 15:35 [PATCH 00/10] config API: make "multi" safe, fix numerous segfaults Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 01/10] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2022-10-26 18:49   ` SZEDER Gábor
2022-10-26 19:33     ` Ævar Arnfjörð Bjarmason
2022-10-27 19:27   ` Junio C Hamano
2022-10-26 15:35 ` [PATCH 02/10] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 03/10] config API: mark *_multi() with RESULT_MUST_BE_USED Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 04/10] string-list API: mark "struct_string_list" to "for_each_string_list" const Ævar Arnfjörð Bjarmason
2022-10-27 19:32   ` Junio C Hamano
2022-10-27 23:04     ` Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 05/10] string-list API: make has_string() and list_lookup() "const" Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 06/10] builtin/gc.c: use "unsorted_string_list_has_string()" where appropriate Ævar Arnfjörð Bjarmason
2022-10-27 19:37   ` Junio C Hamano
2022-10-27 23:25     ` Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 07/10] config API: add and use "lookup_value" functions Ævar Arnfjörð Bjarmason
2022-10-27 19:42   ` Junio C Hamano
2022-10-26 15:35 ` [PATCH 08/10] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2022-10-27 19:43   ` Junio C Hamano
2022-10-26 15:35 ` [PATCH 09/10] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2022-10-27 19:49   ` Junio C Hamano
2022-10-27 19:52     ` Junio C Hamano
2022-10-27 23:44       ` Ævar Arnfjörð Bjarmason
2022-10-28 19:16         ` Junio C Hamano
2022-10-31 18:22           ` Ævar Arnfjörð Bjarmason
2022-10-26 15:35 ` [PATCH 10/10] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2022-10-27 20:12 ` [PATCH 00/10] config API: make "multi" safe, fix numerous segfaults Junio C Hamano
2022-11-01 23:05 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 1/9] for-each-repo tests: test bad --config keys Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 2/9] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 3/9] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 4/9] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 5/9] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 6/9] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 7/9] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 8/9] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2022-11-01 23:05   ` [PATCH v2 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2022-11-02  0:49   ` [PATCH v2 0/9] config API: make "multi" safe, fix numerous segfaults Taylor Blau
2022-11-25  9:50   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-11-25  9:50     ` [PATCH v3 1/9] for-each-repo tests: test bad --config keys Ævar Arnfjörð Bjarmason
2022-11-25  9:50     ` [PATCH v3 2/9] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2023-01-19  0:15       ` Glen Choo
2022-11-25  9:50     ` [PATCH v3 3/9] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2023-01-19  0:28       ` Glen Choo
2022-11-25  9:50     ` [PATCH v3 4/9] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2022-11-25  9:50     ` [PATCH v3 5/9] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2023-01-19  0:50       ` Glen Choo
2022-11-25  9:50     ` [PATCH v3 6/9] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2022-11-25  9:50     ` [PATCH v3 7/9] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2023-01-19  0:51       ` Glen Choo
2022-11-25  9:50     ` [PATCH v3 8/9] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2023-01-19  1:03       ` Glen Choo
2022-11-25  9:50     ` [PATCH v3 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2023-01-19  0:10     ` [PATCH v3 0/9] config API: make "multi" safe, fix numerous segfaults Glen Choo
2023-02-02 13:27     ` [PATCH v4 " Ævar Arnfjörð Bjarmason
2023-02-02 13:27       ` [PATCH v4 1/9] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2023-02-03  1:22         ` Junio C Hamano
2023-02-06  8:31         ` Glen Choo
2023-02-02 13:27       ` [PATCH v4 2/9] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2023-02-02 23:12         ` Junio C Hamano
2023-02-06 10:40         ` Glen Choo
2023-02-06 12:31           ` Ævar Arnfjörð Bjarmason
2023-02-06 16:23             ` Glen Choo
2023-02-02 13:27       ` [PATCH v4 3/9] config API: add and use a "git_config_get()" family of functions Ævar Arnfjörð Bjarmason
2023-02-02 23:56         ` Junio C Hamano
2023-02-07 10:29           ` Ævar Arnfjörð Bjarmason
2023-02-06 12:36         ` Glen Choo
2023-02-06 12:37         ` Glen Choo
2023-02-07 11:52           ` Ævar Arnfjörð Bjarmason
2023-02-02 13:27       ` [PATCH v4 4/9] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2023-02-03 21:52         ` Junio C Hamano
2023-02-02 13:27       ` [PATCH v4 5/9] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2023-02-02 13:27       ` [PATCH v4 6/9] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2023-02-06 12:56         ` Glen Choo
2023-02-02 13:27       ` [PATCH v4 7/9] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2023-02-02 13:27       ` [PATCH v4 8/9] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2023-02-06 13:04         ` Glen Choo
2023-02-02 13:27       ` [PATCH v4 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2023-02-07 16:10       ` [PATCH v5 00/10] config API: make "multi" safe, fix segfaults, propagate "ret" Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 01/10] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 02/10] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2023-02-09  4:00           ` Glen Choo
2023-02-07 16:10         ` [PATCH v5 03/10] config API: add and use a "git_config_get()" family of functions Ævar Arnfjörð Bjarmason
2023-02-09  8:24           ` Glen Choo
2023-02-09 10:11             ` Ævar Arnfjörð Bjarmason
2023-02-09 10:59               ` Ævar Arnfjörð Bjarmason
2023-02-09 16:53                 ` Glen Choo
2023-02-07 16:10         ` [PATCH v5 04/10] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 05/10] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 06/10] config API: don't lose the git_*get*() return values Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 07/10] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 08/10] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 09/10] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2023-02-07 16:10         ` [PATCH v5 10/10] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2023-02-07 17:38         ` [PATCH v5 00/10] config API: make "multi" safe, fix segfaults, propagate "ret" Junio C Hamano
2023-03-07 18:09         ` [PATCH v6 0/9] " Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 1/9] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 2/9] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 3/9] config API: add and use a "git_config_get()" family of functions Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 4/9] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 5/9] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 6/9] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 7/9] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 8/9] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2023-03-07 18:09           ` [PATCH v6 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2023-03-08  0:48           ` [PATCH v6 0/9] config API: make "multi" safe, fix segfaults, propagate "ret" Glen Choo
2023-03-08  9:06           ` [PATCH v7 " Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 1/9] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 2/9] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 3/9] config API: add and use a "git_config_get()" family of functions Ævar Arnfjörð Bjarmason
2023-03-09 18:53               ` Glen Choo
2023-03-14 11:21                 ` Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 4/9] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 5/9] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2023-03-09 19:01               ` Glen Choo
2023-03-08  9:06             ` [PATCH v7 6/9] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 7/9] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 8/9] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2023-03-08  9:06             ` [PATCH v7 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2023-03-09 19:08             ` [PATCH v7 0/9] config API: make "multi" safe, fix segfaults, propagate "ret" Glen Choo
2023-03-09 20:46               ` Junio C Hamano
2023-03-28 14:04             ` [PATCH v8 " Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 1/9] config tests: cover blind spots in git_die_config() tests Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 2/9] config tests: add "NULL" tests for *_get_value_multi() Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 3/9] config API: add and use a "git_config_get()" family of functions Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 4/9] versioncmp.c: refactor config reading next commit Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 5/9] config API: have *_multi() return an "int" and take a "dest" Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 6/9] for-each-repo: error on bad --config Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 7/9] config API users: test for *_get_value_multi() segfaults Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 8/9] config API: add "string" version of *_value_multi(), fix segfaults Ævar Arnfjörð Bjarmason
2023-03-28 14:04               ` [PATCH v8 9/9] for-each-repo: with bad config, don't conflate <path> and <cmd> Ævar Arnfjörð Bjarmason
2023-04-07 15:51                 ` SZEDER Gábor
2023-03-28 16:58               ` [PATCH v8 0/9] config API: make "multi" safe, fix segfaults, propagate "ret" Glen Choo
2023-03-28 17:02                 ` Junio C Hamano
2023-03-29 22:17               ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).