Re: [PATCH] config.c: NULL check when reading protected config

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:

> From: Glen Choo <chooglen@google.com>
>
> In read_protected_config(), check whether each file name is NULL before
> attempting to read it. This mirrors do_git_config_sequence() (which
> read_protected_config() is modelled after).
>
> Without these NULL checks,
>
>  make SANITIZE=address test T=t0410*.sh
>
> fails because xdg_config is NULL, causing us to call fopen(NULL).

FWIW a lot more than that fails, that's just the test I focused on for
the bug report, the others ones (I didn't check out all of them) all
variants of that.

See https://github.com/avar/git/runs/7519070124?check_suite_focus=true
for the current failing run with that "[2]" patch you quoted. We fail a
total of 14 test files (and many more tests within those files).

> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>     config.c: NULL check when reading protected config
>     
>     This fixes the SANITIZE=address failure on master, That was introduced
>     by gc/bare-repo-discovery. Thanks again to Ævar for the original report
>     [1] and for proposing a way to catch this in CI [2].
>     
>     [1]
>     https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com
>     [2]
>     https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1299%2Fchooglen%2Fconfig%2Ffix-sanitize-address-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v1
> Pull-Request: https://github.com/git/git/pull/1299
>
>  config.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/config.c b/config.c
> index 015bec360f5..b0ba7f439a4 100644
> --- a/config.c
> +++ b/config.c
> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)
>  	system_config = git_system_config();
>  	git_global_config(&user_config, &xdg_config);
>  
> -	git_configset_add_file(&protected_config, system_config);
> -	git_configset_add_file(&protected_config, xdg_config);
> -	git_configset_add_file(&protected_config, user_config);
> +	if (system_config)
> +		git_configset_add_file(&protected_config, system_config);
> +	if (xdg_config)
> +		git_configset_add_file(&protected_config, xdg_config);
> +	if (user_config)
> +		git_configset_add_file(&protected_config, user_config);
>  	git_configset_add_parameters(&protected_config);
>  
>  	free(system_config);
>
> base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25

Re your claim in
https://lore.kernel.org/git/kl6lzggwsyh1.fsf@chooglen-macbookpro.roam.corp.google.com/
I tried testing this, and came up with the below.

I wonder if we should work in here for general paranoia, but I'm not too
familiar with the this part of the config reading, maybe we're confident
enough that these are invariants within the process.

This will BUG() out if these variables change within the process, which
would mean that our caching assumptions are no longer true, which would
cause you to return the wrong data here.

Of course you'd have segfaulted or similar before, but this should
demonstrate that not only are these sometimes NULL, but that they stay
that way.

diff --git a/config.c b/config.c
index 015bec360f5..cdd665c1cc8 100644
--- a/config.c
+++ b/config.c
@@ -2102,6 +2102,30 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
+static char *last_system_config;
+static char *last_xdg_config;
+static char *last_user_config;
+
+static void sanity_check_config_1(const char *name, char **last,
+				  const char *now)
+{
+	if (*last && now && strcmp(*last, now))
+		BUG("%s_config: had '%s', now '%s'", name, *last, now);
+	else if (*last && !now)
+		BUG("%s_config: had '%s', now NULL", name, *last);
+	free(*last);
+	*last = xstrdup_or_null(now);
+}
+
+static void sanity_check_config(const char *system_config,
+				const char *xdg_config,
+				const char *user_config)
+{
+	sanity_check_config_1("system", &last_system_config, system_config);
+	sanity_check_config_1("xdg", &last_xdg_config, xdg_config);
+	sanity_check_config_1("user", &last_user_config, user_config);
+}
+
 static int do_git_config_sequence(const struct config_options *opts,
 				  config_fn_t fn, void *data)
 {
@@ -2134,6 +2158,8 @@ static int do_git_config_sequence(const struct config_options *opts,
 	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
 		ret += git_config_from_file(fn, user_config, data);
 
+	sanity_check_config(system_config, xdg_config, user_config);
+
 	current_parsing_scope = CONFIG_SCOPE_LOCAL;
 	if (!opts->ignore_repo && repo_config &&
 	    !access_or_die(repo_config, R_OK, 0))
@@ -2645,6 +2671,8 @@ static void read_protected_config(void)
 	system_config = git_system_config();
 	git_global_config(&user_config, &xdg_config);
 
+	sanity_check_config(system_config, xdg_config, user_config);
+
 	git_configset_add_file(&protected_config, system_config);
 	git_configset_add_file(&protected_config, xdg_config);
 	git_configset_add_file(&protected_config, user_config);