config.c: NULL check when reading protected config

From: Glen Choo <[email protected]>

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

config.c +0 -0

index 015bec360f5..b0ba7f439a4 100644
@@ -2645,9 +2645,12 @@ static void read_protected_config(void)
 	system_config = git_system_config();
 	git_global_config(&user_config, &xdg_config);
 

Quoted 5 lines · Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>

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).

s/modelled/modeled

Quoted 3 lines

Without these NULL checks,

make SANITIZE=address test T=t0410*.sh

I'm glad that t0410 was catching this for us already, though it is too bad we didn't see it outside of the ASan builds, or I think we could have potentially caught this earlier.

Either way, I think the test coverage here is sufficient, so what you wrote makes sense.

Quoted 8 lines

config.c +0 -0

index 015bec360f5..b0ba7f439a4 100644
@@ -2645,9 +2645,12 @@ static void read_protected_config(void)
 	system_config = git_system_config();
 	git_global_config(&user_config, &xdg_config);
 

config.c static void read_protected_config(void) · line 2654

Quoted 10 lines

config.c +6 -3

-	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);

I wonder: should it become a BUG() to call git_configset_add_file() with a NULL filename? That would have elevated the test failure outside of just the ASAn builds, I'd think.

There's certainty a risk of being too defensive, but elevating this error beyond just the ASan builds indicates that this would be an appropriate layer of defense IMHO.

Thanks,
Taylor

Quoted 7 lines

I wonder: should it become a BUG() to call git_configset_add_file() with a NULL filename? That would have elevated the test failure outside of just the ASAn builds, I'd think.

There's certainty a risk of being too defensive, but elevating this error beyond just the ASan builds indicates that this would be an appropriate layer of defense IMHO.

Hm, if we're going in this direction, what if we made it a BUG() to call fopen_or_warn() with a NULL filename? Then we wouldn't have to reimplement this BUG() check in all of its callers.

Quoted 8 lines · Glen Choo

Quoted 7 lines

I wonder: should it become a BUG() to call git_configset_add_file() with a NULL filename? That would have elevated the test failure outside of just the ASAn builds, I'd think.

There's certainty a risk of being too defensive, but elevating this error beyond just the ASan builds indicates that this would be an appropriate layer of defense IMHO.

Quoted 3 lines

Hm, if we're going in this direction, what if we made it a BUG() to call fopen_or_warn() with a NULL filename? Then we wouldn't have to reimplement this BUG() check in all of its callers.

That may be too low-level of a place to put this check, but I don't have a strong opinion about it either way (including whether we should have such a BUG() *anywhere* in this series, including git_configset_add_file()).

From: Glen Choo <[email protected]>

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).

Reported-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Glen Choo <[email protected]>

---
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/[email protected]
[2]
https://lore.kernel.org/git/[email protected]

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(-)

config.c +6 -3

index 015bec360f5..b0ba7f439a4 100644
@@ -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

Quoted 9 lines · Taylor Blau

Quoted 8 lines · Glen Choo

Quoted 7 lines

I wonder: should it become a BUG() to call git_configset_add_file() with a NULL filename? That would have elevated the test failure outside of just the ASAn builds, I'd think.

There's certainty a risk of being too defensive, but elevating this error beyond just the ASan builds indicates that this would be an appropriate layer of defense IMHO.

Quoted 8 lines

Quoted 3 lines

Hm, if we're going in this direction, what if we made it a BUG() to call fopen_or_warn() with a NULL filename? Then we wouldn't have to reimplement this BUG() check in all of its callers.

That may be too low-level of a place to put this check, but I don't have a strong opinion about it either way (including whether we should have such a BUG() *anywhere* in this series, including git_configset_add_file()).

Since git_configset_add_file() returns an 'int', could we return -1 if the supplied 'filename' was null? (The correct place to check would be down in git_config_from_file_with_options().)

It would save all these checks here.

(Also: do we care that we are ignoring the return values in read_protected_config()?

Thanks,
-Stolee

Quoted 11 lines · Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>

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).

Quoted 42 lines

Reported-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Glen Choo <[email protected]>

---
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/[email protected]
[2]
https://lore.kernel.org/git/[email protected]

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(-)

config.c +6 -3

index 015bec360f5..b0ba7f439a4 100644
@@ -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);

Quoted 1 line

base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25

Re your claim in
https://lore.kernel.org/git/[email protected]/
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.

config.c +28 -0

index 015bec360f5..cdd665c1cc8 100644
@@ -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);

Quoted 14 lines · Ævar Arnfjörð Bjarmason

Quoted 11 lines · Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>

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).

Quoted 7 lines

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).

Ah thanks, I'll amend the message accordingly.

Quoted 52 lines

Quoted 42 lines

Reported-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Glen Choo <[email protected]>

---
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/[email protected]
[2]
https://lore.kernel.org/git/[email protected]

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(-)

config.c +6 -3

index 015bec360f5..b0ba7f439a4 100644
@@ -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);

Quoted 1 line

base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25

Re your claim in
https://lore.kernel.org/git/[email protected]/
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.

Quoted 7 lines

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.

Interesting, this is worth proposing, but I suspect that the conversation will be long enough for this to be its own thread. Surely someone must have given some thought to this, especially for long-lived processes (git-daemon?).

There's also the general question of config cache freshness, e.g. what if another git process writes to a shared config file? (We don't worry about the single process case because the process will invalidate its own cache).

Perhaps we should also worry about that (probably more common) case in addition to this one? At any rate, that seems like a bigger topic than this fix here.

config.c +0 -0

index 015bec360f5..e8ebef77d5c 100644
@@ -2645,9 +2647,12 @@ static void read_protected_config(void)
 	system_config = git_system_config();
 	git_global_config(&user_config, &xdg_config);
 

config.c +0 -0

 	f = fopen_or_warn(filename, "r");
 	if (f) {
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,

It however makes me wonder if it is simpler to allow passing NULL to git_config_from_file_with_options() and make it silently turn into a no-op. I.e. instead of ...

config.c int git_config_from_file_with_options(config_fn_t fn, const char *filename, · line 1983

Quoted 6 lines

config.c +2 -0

@@ -1979,6 +1979,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,
 	int ret = -1;
 	FILE *f;
 
+	if (!filename)
+		BUG("filename cannot be NULL");

... we could do

if (!filename)
return 0; /* successful no-op */

Even if there are codepaths that feed arbitrary pathnames given by the end user, they wouldn't be passing NULL (they may pass an empty string, or a filename that causes fopen() to fail), would they?

But that is something we should leave to a follow-up series, not "oops, we need to fix it now" fix.

Thanks, will queue.

Quoted 4 lines

config.c +0 -0

 	f = fopen_or_warn(filename, "r");
 	if (f) {
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,

config.c int git_config_from_file_with_options(config_fn_t fn, const char *filename, · line 1983

Quoted 4 lines · Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>

config.c +2 -0

+	if (!filename)
+		BUG("filename cannot be NULL");

Looks good, but as an aside I wonder if we wouldn't get better code analysis with "nonnull" for this sort of thing, but we can leave this for now:
https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes