[v2,2/8] config.c: don't assign to "cf_global" directly

Commit Message

Glen Choo March 16, 2023, 12:11 a.m. UTC

From: Glen Choo <[e-mail]>

To make "cf_global" easier to remove, replace all direct assignments to
it with function calls. This refactor has an additional maintainability
benefit: all of these functions were manually implementing stack
pop/push semantics on "struct config_source", so replacing them with
function calls allows us to only implement this logic once.

In this process, perform some now-obvious clean ups:

- Drop some unnecessary "cf_global" assignments in
  populate_remote_urls(). Since it was introduced in 399b198489 (config:
  include file if remote URL matches a glob, 2022-01-18), it has stored
  and restored the value of "cf_global" to ensure that it doesn't get
  accidentally mutated. However, this was never necessary since
  "do_config_from()" already pushes/pops "cf_global" further down the
  call chain.

- Zero out every "struct config_source" with a dedicated initializer.
  This matters because the "struct config_source" is assigned to
  "cf_global" and we later 'pop the stack' by assigning "cf_global =
  cf_global->prev", but "cf_global->prev" could be pointing to
  uninitialized garbage.

  Fortunately, this has never bothered us since we never try to read
  "cf_global" except while iterating through config, in which case,
  "cf_global" is either set to a sensible value (when parsing a file),
  or it is ignored (when iterating a configset). Later in the series,
  zero-ing out memory will also let us enforce the constraint that
  "cf_global" and "current_config_kvi" are never non-NULL together.

Signed-off-by: Glen Choo <[e-mail]>
---
 config.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

Comments

Jonathan Tan March 16, 2023, 9:18 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:
> +static inline void config_reader_push_source(struct config_source *top)
> +{
> +	if (cf_global)
> +		top->prev = cf_global;

Don't we want to set prev unconditionally here (i.e. set it to NULL if
cf_global was NULL)?

> +	cf_global = top;
> +}
> +
> +static inline struct config_source *config_reader_pop_source()
> +{
> +	struct config_source *ret;
> +	if (!cf_global)
> +		BUG("tried to pop config source, but we weren't reading config");
> +	ret = cf_global;
> +	cf_global = cf_global->prev;
> +	return ret;
> +}

...since we use it unconditionally here.

The rest looks good.

Junio C Hamano March 16, 2023, 9:31 p.m. UTC | #2

Jonathan Tan <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>> +static inline void config_reader_push_source(struct config_source *top)
>> +{
>> +	if (cf_global)
>> +		top->prev = cf_global;
>
> Don't we want to set prev unconditionally here (i.e. set it to NULL if
> cf_global was NULL)?

Good eyes.  You are absolutely right.
Thanks.

>
>> +	cf_global = top;
>> +}
>> +
>> +static inline struct config_source *config_reader_pop_source()
>> +{
>> +	struct config_source *ret;
>> +	if (!cf_global)
>> +		BUG("tried to pop config source, but we weren't reading config");
>> +	ret = cf_global;
>> +	cf_global = cf_global->prev;
>> +	return ret;
>> +}
>
> ...since we use it unconditionally here.
>
> The rest looks good.

Glen Choo March 16, 2023, 10:56 p.m. UTC | #3

Jonathan Tan <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>> +static inline void config_reader_push_source(struct config_source *top)
>> +{
>> +	if (cf_global)
>> +		top->prev = cf_global;
>
> Don't we want to set prev unconditionally here (i.e. set it to NULL if
> cf_global was NULL)?
>
>> +	cf_global = top;
>> +}
>> +
>> +static inline struct config_source *config_reader_pop_source()
>> +{
>> +	struct config_source *ret;
>> +	if (!cf_global)
>> +		BUG("tried to pop config source, but we weren't reading config");
>> +	ret = cf_global;
>> +	cf_global = cf_global->prev;
>> +	return ret;
>> +}
>
> ...since we use it unconditionally here.

Ah, thanks. It's safe as of this patch, since we always zero-initialize
"struct config_source" and nothing else sets ".prev", but this asymmetry
is a bug waiting to happen.

Patch

diff --git a/config.c b/config.c
index e4a76739365..517b8f64038 100644
--- a/config.c
+++ b/config.c
@@ -49,6 +49,7 @@  struct config_source {
 	int (*do_ungetc)(int c, struct config_source *conf);
 	long (*do_ftell)(struct config_source *c);
 };
+#define CONFIG_SOURCE_INIT { 0 }
 
 /*
  * These variables record the "current" config source, which
@@ -79,6 +80,23 @@  static struct key_value_info *current_config_kvi;
  */
 static enum config_scope current_parsing_scope;
 
+static inline void config_reader_push_source(struct config_source *top)
+{
+	if (cf_global)
+		top->prev = cf_global;
+	cf_global = top;
+}
+
+static inline struct config_source *config_reader_pop_source()
+{
+	struct config_source *ret;
+	if (!cf_global)
+		BUG("tried to pop config source, but we weren't reading config");
+	ret = cf_global;
+	cf_global = cf_global->prev;
+	return ret;
+}
+
 static int pack_compression_seen;
 static int zlib_compression_seen;
 
@@ -346,14 +364,12 @@  static void populate_remote_urls(struct config_include_data *inc)
 {
 	struct config_options opts;
 
-	struct config_source *store_cf = cf_global;
 	struct key_value_info *store_kvi = current_config_kvi;
 	enum config_scope store_scope = current_parsing_scope;
 
 	opts = *inc->opts;
 	opts.unconditional_remote_url = 1;
 
-	cf_global = NULL;
 	current_config_kvi = NULL;
 	current_parsing_scope = 0;
 
@@ -361,7 +377,6 @@  static void populate_remote_urls(struct config_include_data *inc)
 	string_list_init_dup(inc->remote_urls);
 	config_with_options(add_remote_url, inc->remote_urls, inc->config_source, &opts);
 
-	cf_global = store_cf;
 	current_config_kvi = store_kvi;
 	current_parsing_scope = store_scope;
 }
@@ -715,12 +730,10 @@  int git_config_from_parameters(config_fn_t fn, void *data)
 	struct strvec to_free = STRVEC_INIT;
 	int ret = 0;
 	char *envw = NULL;
-	struct config_source source;
+	struct config_source source = CONFIG_SOURCE_INIT;
 
-	memset(&source, 0, sizeof(source));
-	source.prev = cf_global;
 	source.origin_type = CONFIG_ORIGIN_CMDLINE;
-	cf_global = &source;
+	config_reader_push_source(&source);
 
 	env = getenv(CONFIG_COUNT_ENVIRONMENT);
 	if (env) {
@@ -778,7 +791,7 @@  out:
 	strbuf_release(&envvar);
 	strvec_clear(&to_free);
 	free(envw);
-	cf_global = source.prev;
+	config_reader_pop_source();
 	return ret;
 }
 
@@ -1949,20 +1962,19 @@  static int do_config_from(struct config_source *top, config_fn_t fn, void *data,
 	int ret;
 
 	/* push config-file parsing state stack */
-	top->prev = cf_global;
 	top->linenr = 1;
 	top->eof = 0;
 	top->total_len = 0;
 	strbuf_init(&top->value, 1024);
 	strbuf_init(&top->var, 1024);
-	cf_global = top;
+	config_reader_push_source(top);
 
 	ret = git_parse_source(top, fn, data, opts);
 
 	/* pop config-file parsing state stack */
 	strbuf_release(&top->value);
 	strbuf_release(&top->var);
-	cf_global = top->prev;
+	config_reader_pop_source();
 
 	return ret;
 }
@@ -1972,7 +1984,7 @@  static int do_config_from_file(config_fn_t fn,
 		const char *name, const char *path, FILE *f,
 		void *data, const struct config_options *opts)
 {
-	struct config_source top;
+	struct config_source top = CONFIG_SOURCE_INIT;
 	int ret;
 
 	top.u.file = f;
@@ -2024,7 +2036,7 @@  int git_config_from_mem(config_fn_t fn,
 			const char *name, const char *buf, size_t len,
 			void *data, const struct config_options *opts)
 {
-	struct config_source top;
+	struct config_source top = CONFIG_SOURCE_INIT;
 
 	top.u.buf.buf = buf;
 	top.u.buf.len = len;