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

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

[Glen Choo via GitGitGadget]

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

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
-- 
gitgitgadget

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

[Taylor Blau]

On Tue, Jul 26, 2022 at 05:09:32PM +0000, 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).

s/modelled/modeled

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

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

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

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

[Glen Choo]

Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Jul 26, 2022 at 05:09:32PM +0000, 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).
>
> s/modelled/modeled

Ah, thanks.

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

>
> Thanks,
> Taylor

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

[Taylor Blau]

On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:
> > 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.

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

Thanks,
Taylor

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

[Derrick Stolee]

On 7/26/2022 1:43 PM, Taylor Blau wrote:
> On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:
>>> 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.
> 
> 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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Glen Choo]

Derrick Stolee <derrickstolee@github.com> writes:

> On 7/26/2022 1:43 PM, Taylor Blau wrote:
>> On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:
>>>> 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.
>> 
>> 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.

Hm, IIUC you are suggesting that git_configset_add_file() returns -1
instead of BUG()-ing?

BUG() sounds better IMO, since there really is nothing useful that
git_configset_add_file() (and later functions) can do with a NULL file
name. Plus, git_configset_add_file() has already reserved -1 to mean
"a file was specified but could not be read".

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

I don't think we care (unless this is a style issue). "git config" 
succeeds even if it encounters non-repo files that can't be read. In a
similar vein, I don't think it matters for protected config if we can't
read one of the files (e.g. xdg_config) or even all of the files; all
that matters is that we've read everything that we can.

>
> Thanks,
> -Stolee

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

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Ah thanks, I'll amend the message accordingly.

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

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.

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

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

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

In read_protected_config(), check whether each file name is NULL before
attempting to read it, and add a BUG() call to
git_config_from_file_with_options() to make this error easier to catch
in the future.

The NULL checks mirror what do_git_config_sequence() does (which
read_protected_config() is modeled after). Without these NULL checks,
multiple tests fail with "make SANITIZE=address", e.g. in the final test
of t4010, xdg_config is NULL causing us to call fopen(NULL).

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].
    
    Changes in v2:
    
     * Fix typo
     * Add BUG() to git_config_from_file_with_options()
    
    [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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v2
Pull-Request: https://github.com/git/git/pull/1299

Range-diff vs v1:

 1:  17b4a489c69 ! 1:  ba51078418a config.c: NULL check when reading protected config
     @@ Commit message
          config.c: NULL check when reading protected config
      
          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).
     +    attempting to read it, and add a BUG() call to
     +    git_config_from_file_with_options() to make this error easier to catch
     +    in the future.
      
     -    Without these NULL checks,
     -
     -     make SANITIZE=address test T=t0410*.sh
     -
     -    fails because xdg_config is NULL, causing us to call fopen(NULL).
     +    The NULL checks mirror what do_git_config_sequence() does (which
     +    read_protected_config() is modeled after). Without these NULL checks,
     +    multiple tests fail with "make SANITIZE=address", e.g. in the final test
     +    of t4010, xdg_config is NULL causing us to call fopen(NULL).
      
          Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
          Signed-off-by: Glen Choo <chooglen@google.com>
      
       ## config.c ##
     +@@ config.c: 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");
     + 	f = fopen_or_warn(filename, "r");
     + 	if (f) {
     + 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
      @@ config.c: static void read_protected_config(void)
       	system_config = git_system_config();
       	git_global_config(&user_config, &xdg_config);


 config.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 015bec360f5..e8ebef77d5c 100644
--- a/config.c
+++ b/config.c
@@ -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");
 	f = fopen_or_warn(filename, "r");
 	if (f) {
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,
@@ -2645,9 +2647,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
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 26 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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).
>
> Ah thanks, I'll amend the message accordingly.
>
>>> 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.
>
> 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.

We can leave it for later, I've run it as a one-off and didn't have any
failures.

But FWIW I think it's tied up in this fix here, i.e. your original code
both added caching, and implicitly assumed that these were never NULL,
so it was "obvious" that it didn't need such assertions.

But now we have 3x if's in a code path that's cached, and the cache is
*not* guarded by the same 3x checks.

So we can leave it for later, but it really seems worth adding some
self-documentation here sooner than later.

This BUG() method I came up with is one way, another would be to strdup
it and use a "static" variable in the function, i.e. stick with whatever
value(s) we start out with.

But in any case, this fix seems correct, and fixes the current issues
SANITIZE=address is spotting for us, thanks!

nonnull v.s. BUG() if !x (was: [PATCH v2] config.c: NULL check when reading protected config)

[Ævar Arnfjörð Bjarmason]

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

> From: Glen Choo <chooglen@google.com>

> +	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

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

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/config.c b/config.c
> index 015bec360f5..e8ebef77d5c 100644
> --- a/config.c
> +++ b/config.c
> @@ -2645,9 +2647,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);

This does make it similar to the way how the primary "config
sequence" reader calls git_config_from_file(), so I do prefer the
patch as posted as a short-term "oops, we merged a buggy code that
segfaults and here is a fix" patch.

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

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

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

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

[Glen Choo]

Junio C Hamano <gitster@pobox.com> writes:

> 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 ...
>
>> @@ -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?

Yeah, that's worth considering. I'm not sure how I feel about it yet,
but hopefully I find some time to dig around and form an opinion.

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

Thanks :)

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

Re: nonnull v.s. BUG() if !x (was: [PATCH v2] config.c: NULL check when reading protected config)

[Glen Choo]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@google.com>
>
>> +	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

Interesting. I wonder how good the analysis is vs the cost, e.g. it's
useful if it detects _maybe_ NULL variables, but it might be too
expensive if it requires us to mark all of our variables as non-NULL.