{"thread":{"id":"58227","subject":"[PATCH] config.c: NULL check when reading protected config","startedAt":"2022-07-26T17:09:39Z","lastAt":"2022-07-27T18:03:40Z","messageCount":14,"participants":["Glen Choo via GitGitGadget","Taylor Blau","Glen Choo","Derrick Stolee","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"459986","messageId":"pull.1299.git.git.1658855372189.gitgitgadget@gmail.com","threadId":"58227","inReplyTo":null,"subject":"[PATCH] config.c: NULL check when reading protected config","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-26T17:09:32Z","receivedAt":"2022-07-26T17:09:39Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nIn read_protected_config(), check whether each file name is NULL before\nattempting to read it. This mirrors do_git_config_sequence() (which\nread_protected_config() is modelled after).\n\nWithout these NULL checks,\n\n make SANITIZE=address test T=t0410*.sh\n\nfails because xdg_config is NULL, causing us to call fopen(NULL).\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n config.c: NULL check when reading protected config\n \n This fixes the SANITIZE=address failure on master, That was introduced\n by gc/bare-repo-discovery. Thanks again to Ævar for the original report\n [1] and for proposing a way to catch this in CI [2].\n \n [1]\n https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com\n [2]\n https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1299%2Fchooglen%2Fconfig%2Ffix-sanitize-address-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v1\nPull-Request: https://github.com/git/git/pull/1299\n\n config.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 015bec360f5..b0ba7f439a4 100644\n--- a/config.c\n+++ b/config.c\n@@ -2645,9 +2645,12 @@ static void read_protected_config(void)\n \tsystem_config = git_system_config();\n \tgit_global_config(&user_config, &xdg_config);\n \n-\tgit_configset_add_file(&protected_config, system_config);\n-\tgit_configset_add_file(&protected_config, xdg_config);\n-\tgit_configset_add_file(&protected_config, user_config);\n+\tif (system_config)\n+\t\tgit_configset_add_file(&protected_config, system_config);\n+\tif (xdg_config)\n+\t\tgit_configset_add_file(&protected_config, xdg_config);\n+\tif (user_config)\n+\t\tgit_configset_add_file(&protected_config, user_config);\n \tgit_configset_add_parameters(&protected_config);\n \n \tfree(system_config);\n\nbase-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n-- \ngitgitgadget\n"},{"id":"459987","messageId":"YuAj+eBajf6jkJPc@nand.local","threadId":"58227","inReplyTo":"pull.1299.git.git.1658855372189.gitgitgadget@gmail.com","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-26T17:27:21Z","receivedAt":"2022-07-26T17:27:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 26, 2022 at 05:09:32PM +0000, Glen Choo via GitGitGadget wrote:\n> From: Glen Choo \n>\n> In read_protected_config(), check whether each file name is NULL before\n> attempting to read it. This mirrors do_git_config_sequence() (which\n> read_protected_config() is modelled after).\n\ns/modelled/modeled\n\n> Without these NULL checks,\n>\n> make SANITIZE=address test T=t0410*.sh\n\nI'm glad that t0410 was catching this for us already, though it is too\nbad we didn't see it outside of the ASan builds, or I think we could\nhave potentially caught this earlier.\n\nEither way, I think the test coverage here is sufficient, so what you\nwrote makes sense.\n\n> diff --git a/config.c b/config.c\n> index 015bec360f5..b0ba7f439a4 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)\n> \tsystem_config = git_system_config();\n> \tgit_global_config(&user_config, &xdg_config);\n>\n> -\tgit_configset_add_file(&protected_config, system_config);\n> -\tgit_configset_add_file(&protected_config, xdg_config);\n> -\tgit_configset_add_file(&protected_config, user_config);\n> +\tif (system_config)\n> +\t\tgit_configset_add_file(&protected_config, system_config);\n> +\tif (xdg_config)\n> +\t\tgit_configset_add_file(&protected_config, xdg_config);\n> +\tif (user_config)\n> +\t\tgit_configset_add_file(&protected_config, user_config);\n> \tgit_configset_add_parameters(&protected_config);\n\nI wonder: should it become a BUG() to call git_configset_add_file() with\na NULL filename? That would have elevated the test failure outside of\njust the ASAn builds, I'd think.\n\nThere's certainty a risk of being too defensive, but elevating this\nerror beyond just the ASan builds indicates that this would be an\nappropriate layer of defense IMHO.\n\nThanks,\nTaylor\n"},{"id":"459988","messageId":"kl6llesfsrgd.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58227","inReplyTo":"YuAj+eBajf6jkJPc@nand.local","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-26T17:40:18Z","receivedAt":"2022-07-26T17:40:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, Jul 26, 2022 at 05:09:32PM +0000, Glen Choo via GitGitGadget wrote:\n>> From: Glen Choo \n>>\n>> In read_protected_config(), check whether each file name is NULL before\n>> attempting to read it. This mirrors do_git_config_sequence() (which\n>> read_protected_config() is modelled after).\n>\n> s/modelled/modeled\n\nAh, thanks.\n\n>> Without these NULL checks,\n>>\n>> make SANITIZE=address test T=t0410*.sh\n>\n> I'm glad that t0410 was catching this for us already, though it is too\n> bad we didn't see it outside of the ASan builds, or I think we could\n> have potentially caught this earlier.\n>\n> Either way, I think the test coverage here is sufficient, so what you\n> wrote makes sense.\n>\n>> diff --git a/config.c b/config.c\n>> index 015bec360f5..b0ba7f439a4 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)\n>> \tsystem_config = git_system_config();\n>> \tgit_global_config(&user_config, &xdg_config);\n>>\n>> -\tgit_configset_add_file(&protected_config, system_config);\n>> -\tgit_configset_add_file(&protected_config, xdg_config);\n>> -\tgit_configset_add_file(&protected_config, user_config);\n>> +\tif (system_config)\n>> +\t\tgit_configset_add_file(&protected_config, system_config);\n>> +\tif (xdg_config)\n>> +\t\tgit_configset_add_file(&protected_config, xdg_config);\n>> +\tif (user_config)\n>> +\t\tgit_configset_add_file(&protected_config, user_config);\n>> \tgit_configset_add_parameters(&protected_config);\n>\n> I wonder: should it become a BUG() to call git_configset_add_file() with\n> a NULL filename? That would have elevated the test failure outside of\n> just the ASAn builds, I'd think.\n>\n> There's certainty a risk of being too defensive, but elevating this\n> error beyond just the ASan builds indicates that this would be an\n> appropriate layer of defense IMHO.\n\nHm, if we're going in this direction, what if we made it a BUG() to call\nfopen_or_warn() with a NULL filename? Then we wouldn't have to\nreimplement this BUG() check in all of its callers.\n\n>\n> Thanks,\n> Taylor\n"},{"id":"459989","messageId":"YuAn171BhecC1w+O@nand.local","threadId":"58227","inReplyTo":"kl6llesfsrgd.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-26T17:43:51Z","receivedAt":"2022-07-26T17:44:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:\n> > I wonder: should it become a BUG() to call git_configset_add_file() with\n> > a NULL filename? That would have elevated the test failure outside of\n> > just the ASAn builds, I'd think.\n> >\n> > There's certainty a risk of being too defensive, but elevating this\n> > error beyond just the ASan builds indicates that this would be an\n> > appropriate layer of defense IMHO.\n>\n> Hm, if we're going in this direction, what if we made it a BUG() to call\n> fopen_or_warn() with a NULL filename? Then we wouldn't have to\n> reimplement this BUG() check in all of its callers.\n\nThat may be too low-level of a place to put this check, but I don't have\na strong opinion about it either way (including whether we should have\nsuch a BUG() *anywhere* in this series, including\ngit_configset_add_file()).\n\nThanks,\nTaylor\n"},{"id":"459990","messageId":"47c0803c-bd98-0460-1e9f-c37dc3deeb8d@github.com","threadId":"58227","inReplyTo":"YuAn171BhecC1w+O@nand.local","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-26T17:51:17Z","receivedAt":"2022-07-26T17:52:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/26/2022 1:43 PM, Taylor Blau wrote:\n> On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:\n>>> I wonder: should it become a BUG() to call git_configset_add_file() with\n>>> a NULL filename? That would have elevated the test failure outside of\n>>> just the ASAn builds, I'd think.\n>>>\n>>> There's certainty a risk of being too defensive, but elevating this\n>>> error beyond just the ASan builds indicates that this would be an\n>>> appropriate layer of defense IMHO.\n>>\n>> Hm, if we're going in this direction, what if we made it a BUG() to call\n>> fopen_or_warn() with a NULL filename? Then we wouldn't have to\n>> reimplement this BUG() check in all of its callers.\n> \n> That may be too low-level of a place to put this check, but I don't have\n> a strong opinion about it either way (including whether we should have\n> such a BUG() *anywhere* in this series, including\n> git_configset_add_file()).\n\nSince git_configset_add_file() returns an 'int', could we return -1\nif the supplied 'filename' was null? (The correct place to check would\nbe down in git_config_from_file_with_options().)\n\nIt would save all these checks here.\n\n(Also: do we care that we are ignoring the return values in\nread_protected_config()?\n\nThanks,\n-Stolee\n"},{"id":"459997","messageId":"220726.864jz3n0uf.gmgdl@evledraar.gmail.com","threadId":"58227","inReplyTo":"pull.1299.git.git.1658855372189.gitgitgadget@gmail.com","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-26T19:03:00Z","receivedAt":"2022-07-26T19:14:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> In read_protected_config(), check whether each file name is NULL before\n> attempting to read it. This mirrors do_git_config_sequence() (which\n> read_protected_config() is modelled after).\n>\n> Without these NULL checks,\n>\n> make SANITIZE=address test T=t0410*.sh\n>\n> fails because xdg_config is NULL, causing us to call fopen(NULL).\n\nFWIW a lot more than that fails, that's just the test I focused on for\nthe bug report, the others ones (I didn't check out all of them) all\nvariants of that.\n\nSee https://github.com/avar/git/runs/7519070124?check_suite_focus=true\nfor the current failing run with that \"[2]\" patch you quoted. We fail a\ntotal of 14 test files (and many more tests within those files).\n\n> Reported-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Glen Choo \n> ---\n> config.c: NULL check when reading protected config\n> \n> This fixes the SANITIZE=address failure on master, That was introduced\n> by gc/bare-repo-discovery. Thanks again to Ævar for the original report\n> [1] and for proposing a way to catch this in CI [2].\n> \n> [1]\n> https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com\n> [2]\n> https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1299%2Fchooglen%2Fconfig%2Ffix-sanitize-address-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v1\n> Pull-Request: https://github.com/git/git/pull/1299\n>\n> config.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 015bec360f5..b0ba7f439a4 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)\n> \tsystem_config = git_system_config();\n> \tgit_global_config(&user_config, &xdg_config);\n> \n> -\tgit_configset_add_file(&protected_config, system_config);\n> -\tgit_configset_add_file(&protected_config, xdg_config);\n> -\tgit_configset_add_file(&protected_config, user_config);\n> +\tif (system_config)\n> +\t\tgit_configset_add_file(&protected_config, system_config);\n> +\tif (xdg_config)\n> +\t\tgit_configset_add_file(&protected_config, xdg_config);\n> +\tif (user_config)\n> +\t\tgit_configset_add_file(&protected_config, user_config);\n> \tgit_configset_add_parameters(&protected_config);\n> \n> \tfree(system_config);\n>\n> base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n\nRe your claim in\nhttps://lore.kernel.org/git/kl6lzggwsyh1.fsf@chooglen-macbookpro.roam.corp.google.com/\nI tried testing this, and came up with the below.\n\nI wonder if we should work in here for general paranoia, but I'm not too\nfamiliar with the this part of the config reading, maybe we're confident\nenough that these are invariants within the process.\n\nThis will BUG() out if these variables change within the process, which\nwould mean that our caching assumptions are no longer true, which would\ncause you to return the wrong data here.\n\nOf course you'd have segfaulted or similar before, but this should\ndemonstrate that not only are these sometimes NULL, but that they stay\nthat way.\n\ndiff --git a/config.c b/config.c\nindex 015bec360f5..cdd665c1cc8 100644\n--- a/config.c\n+++ b/config.c\n@@ -2102,6 +2102,30 @@ int git_config_system(void)\n \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n }\n \n+static char *last_system_config;\n+static char *last_xdg_config;\n+static char *last_user_config;\n+\n+static void sanity_check_config_1(const char *name, char **last,\n+\t\t\t\t const char *now)\n+{\n+\tif (*last && now && strcmp(*last, now))\n+\t\tBUG(\"%s_config: had '%s', now '%s'\", name, *last, now);\n+\telse if (*last && !now)\n+\t\tBUG(\"%s_config: had '%s', now NULL\", name, *last);\n+\tfree(*last);\n+\t*last = xstrdup_or_null(now);\n+}\n+\n+static void sanity_check_config(const char *system_config,\n+\t\t\t\tconst char *xdg_config,\n+\t\t\t\tconst char *user_config)\n+{\n+\tsanity_check_config_1(\"system\", &last_system_config, system_config);\n+\tsanity_check_config_1(\"xdg\", &last_xdg_config, xdg_config);\n+\tsanity_check_config_1(\"user\", &last_user_config, user_config);\n+}\n+\n static int do_git_config_sequence(const struct config_options *opts,\n \t\t\t\t config_fn_t fn, void *data)\n {\n@@ -2134,6 +2158,8 @@ static int do_git_config_sequence(const struct config_options *opts,\n \tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file(fn, user_config, data);\n \n+\tsanity_check_config(system_config, xdg_config, user_config);\n+\n \tcurrent_parsing_scope = CONFIG_SCOPE_LOCAL;\n \tif (!opts->ignore_repo && repo_config &&\n \t !access_or_die(repo_config, R_OK, 0))\n@@ -2645,6 +2671,8 @@ static void read_protected_config(void)\n \tsystem_config = git_system_config();\n \tgit_global_config(&user_config, &xdg_config);\n \n+\tsanity_check_config(system_config, xdg_config, user_config);\n+\n \tgit_configset_add_file(&protected_config, system_config);\n \tgit_configset_add_file(&protected_config, xdg_config);\n \tgit_configset_add_file(&protected_config, user_config);\n"},{"id":"459999","messageId":"kl6lilnjslsb.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58227","inReplyTo":"47c0803c-bd98-0460-1e9f-c37dc3deeb8d@github.com","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-26T19:42:44Z","receivedAt":"2022-07-26T19:43:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Derrick Stolee writes:\n\n> On 7/26/2022 1:43 PM, Taylor Blau wrote:\n>> On Tue, Jul 26, 2022 at 10:40:18AM -0700, Glen Choo wrote:\n>>>> I wonder: should it become a BUG() to call git_configset_add_file() with\n>>>> a NULL filename? That would have elevated the test failure outside of\n>>>> just the ASAn builds, I'd think.\n>>>>\n>>>> There's certainty a risk of being too defensive, but elevating this\n>>>> error beyond just the ASan builds indicates that this would be an\n>>>> appropriate layer of defense IMHO.\n>>>\n>>> Hm, if we're going in this direction, what if we made it a BUG() to call\n>>> fopen_or_warn() with a NULL filename? Then we wouldn't have to\n>>> reimplement this BUG() check in all of its callers.\n>> \n>> That may be too low-level of a place to put this check, but I don't have\n>> a strong opinion about it either way (including whether we should have\n>> such a BUG() *anywhere* in this series, including\n>> git_configset_add_file()).\n>\n> Since git_configset_add_file() returns an 'int', could we return -1\n> if the supplied 'filename' was null? (The correct place to check would\n> be down in git_config_from_file_with_options().)\n>\n> It would save all these checks here.\n\nHm, IIUC you are suggesting that git_configset_add_file() returns -1\ninstead of BUG()-ing?\n\nBUG() sounds better IMO, since there really is nothing useful that\ngit_configset_add_file() (and later functions) can do with a NULL file\nname. Plus, git_configset_add_file() has already reserved -1 to mean\n\"a file was specified but could not be read\".\n\n>\n> (Also: do we care that we are ignoring the return values in\n> read_protected_config()?\n\nI don't think we care (unless this is a style issue). \"git config\" \nsucceeds even if it encounters non-repo files that can't be read. In a\nsimilar vein, I don't think it matters for protected config if we can't\nread one of the files (e.g. xdg_config) or even all of the files; all\nthat matters is that we've read everything that we can.\n\n>\n> Thanks,\n> -Stolee\n"},{"id":"460001","messageId":"kl6lfsinskzm.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58227","inReplyTo":"220726.864jz3n0uf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-26T19:59:57Z","receivedAt":"2022-07-26T20:00:02Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:\n>\n>> From: Glen Choo \n>>\n>> In read_protected_config(), check whether each file name is NULL before\n>> attempting to read it. This mirrors do_git_config_sequence() (which\n>> read_protected_config() is modelled after).\n>>\n>> Without these NULL checks,\n>>\n>> make SANITIZE=address test T=t0410*.sh\n>>\n>> fails because xdg_config is NULL, causing us to call fopen(NULL).\n>\n> FWIW a lot more than that fails, that's just the test I focused on for\n> the bug report, the others ones (I didn't check out all of them) all\n> variants of that.\n>\n> See https://github.com/avar/git/runs/7519070124?check_suite_focus=true\n> for the current failing run with that \"[2]\" patch you quoted. We fail a\n> total of 14 test files (and many more tests within those files).\n\nAh thanks, I'll amend the message accordingly.\n\n>> Reported-by: Ævar Arnfjörð Bjarmason \n>> Signed-off-by: Glen Choo \n>> ---\n>> config.c: NULL check when reading protected config\n>> \n>> This fixes the SANITIZE=address failure on master, That was introduced\n>> by gc/bare-repo-discovery. Thanks again to Ævar for the original report\n>> [1] and for proposing a way to catch this in CI [2].\n>> \n>> [1]\n>> https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com\n>> [2]\n>> https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1299%2Fchooglen%2Fconfig%2Ffix-sanitize-address-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v1\n>> Pull-Request: https://github.com/git/git/pull/1299\n>>\n>> config.c | 9 ++++++---\n>> 1 file changed, 6 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/config.c b/config.c\n>> index 015bec360f5..b0ba7f439a4 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)\n>> \tsystem_config = git_system_config();\n>> \tgit_global_config(&user_config, &xdg_config);\n>> \n>> -\tgit_configset_add_file(&protected_config, system_config);\n>> -\tgit_configset_add_file(&protected_config, xdg_config);\n>> -\tgit_configset_add_file(&protected_config, user_config);\n>> +\tif (system_config)\n>> +\t\tgit_configset_add_file(&protected_config, system_config);\n>> +\tif (xdg_config)\n>> +\t\tgit_configset_add_file(&protected_config, xdg_config);\n>> +\tif (user_config)\n>> +\t\tgit_configset_add_file(&protected_config, user_config);\n>> \tgit_configset_add_parameters(&protected_config);\n>> \n>> \tfree(system_config);\n>>\n>> base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n>\n> Re your claim in\n> https://lore.kernel.org/git/kl6lzggwsyh1.fsf@chooglen-macbookpro.roam.corp.google.com/\n> I tried testing this, and came up with the below.\n>\n> I wonder if we should work in here for general paranoia, but I'm not too\n> familiar with the this part of the config reading, maybe we're confident\n> enough that these are invariants within the process.\n>\n> This will BUG() out if these variables change within the process, which\n> would mean that our caching assumptions are no longer true, which would\n> cause you to return the wrong data here.\n>\n> Of course you'd have segfaulted or similar before, but this should\n> demonstrate that not only are these sometimes NULL, but that they stay\n> that way.\n\nInteresting, this is worth proposing, but I suspect that the\nconversation will be long enough for this to be its own thread. Surely\nsomeone must have given some thought to this, especially for long-lived\nprocesses (git-daemon?).\n\nThere's also the general question of config cache freshness, e.g. what\nif another git process writes to a shared config file? (We don't worry\nabout the single process case because the process will invalidate its\nown cache).\n\nPerhaps we should also worry about that (probably more common) case in\naddition to this one? At any rate, that seems like a bigger topic than\nthis fix here.\n\n>\n> diff --git a/config.c b/config.c\n> index 015bec360f5..cdd665c1cc8 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2102,6 +2102,30 @@ int git_config_system(void)\n> \treturn !git_env_bool(\"GIT_CONFIG_NOSYSTEM\", 0);\n> }\n> \n> +static char *last_system_config;\n> +static char *last_xdg_config;\n> +static char *last_user_config;\n> +\n> +static void sanity_check_config_1(const char *name, char **last,\n> +\t\t\t\t const char *now)\n> +{\n> +\tif (*last && now && strcmp(*last, now))\n> +\t\tBUG(\"%s_config: had '%s', now '%s'\", name, *last, now);\n> +\telse if (*last && !now)\n> +\t\tBUG(\"%s_config: had '%s', now NULL\", name, *last);\n> +\tfree(*last);\n> +\t*last = xstrdup_or_null(now);\n> +}\n> +\n> +static void sanity_check_config(const char *system_config,\n> +\t\t\t\tconst char *xdg_config,\n> +\t\t\t\tconst char *user_config)\n> +{\n> +\tsanity_check_config_1(\"system\", &last_system_config, system_config);\n> +\tsanity_check_config_1(\"xdg\", &last_xdg_config, xdg_config);\n> +\tsanity_check_config_1(\"user\", &last_user_config, user_config);\n> +}\n> +\n> static int do_git_config_sequence(const struct config_options *opts,\n> \t\t\t\t config_fn_t fn, void *data)\n> {\n> @@ -2134,6 +2158,8 @@ static int do_git_config_sequence(const struct config_options *opts,\n> \tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))\n> \t\tret += git_config_from_file(fn, user_config, data);\n> \n> +\tsanity_check_config(system_config, xdg_config, user_config);\n> +\n> \tcurrent_parsing_scope = CONFIG_SCOPE_LOCAL;\n> \tif (!opts->ignore_repo && repo_config &&\n> \t !access_or_die(repo_config, R_OK, 0))\n> @@ -2645,6 +2671,8 @@ static void read_protected_config(void)\n> \tsystem_config = git_system_config();\n> \tgit_global_config(&user_config, &xdg_config);\n> \n> +\tsanity_check_config(system_config, xdg_config, user_config);\n> +\n> \tgit_configset_add_file(&protected_config, system_config);\n> \tgit_configset_add_file(&protected_config, xdg_config);\n> \tgit_configset_add_file(&protected_config, user_config);\n"},{"id":"460005","messageId":"pull.1299.v2.git.git.1658874067077.gitgitgadget@gmail.com","threadId":"58227","inReplyTo":"pull.1299.git.git.1658855372189.gitgitgadget@gmail.com","subject":"[PATCH v2] config.c: NULL check when reading protected config","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-26T22:21:06Z","receivedAt":"2022-07-26T22:21:16Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\nIn read_protected_config(), check whether each file name is NULL before\nattempting to read it, and add a BUG() call to\ngit_config_from_file_with_options() to make this error easier to catch\nin the future.\n\nThe NULL checks mirror what do_git_config_sequence() does (which\nread_protected_config() is modeled after). Without these NULL checks,\nmultiple tests fail with \"make SANITIZE=address\", e.g. in the final test\nof t4010, xdg_config is NULL causing us to call fopen(NULL).\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Glen Choo \n---\n config.c: NULL check when reading protected config\n \n This fixes the SANITIZE=address failure on master, That was introduced\n by gc/bare-repo-discovery. Thanks again to Ævar for the original report\n [1] and for proposing a way to catch this in CI [2].\n \n Changes in v2:\n \n * Fix typo\n * Add BUG() to git_config_from_file_with_options()\n \n [1]\n https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com\n [2]\n https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1299%2Fchooglen%2Fconfig%2Ffix-sanitize-address-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v2\nPull-Request: https://github.com/git/git/pull/1299\n\nRange-diff vs v1:\n\n 1: 17b4a489c69 ! 1: ba51078418a config.c: NULL check when reading protected config\n @@ Commit message\n config.c: NULL check when reading protected config\n \n In read_protected_config(), check whether each file name is NULL before\n - attempting to read it. This mirrors do_git_config_sequence() (which\n - read_protected_config() is modelled after).\n + attempting to read it, and add a BUG() call to\n + git_config_from_file_with_options() to make this error easier to catch\n + in the future.\n \n - Without these NULL checks,\n -\n - make SANITIZE=address test T=t0410*.sh\n -\n - fails because xdg_config is NULL, causing us to call fopen(NULL).\n + The NULL checks mirror what do_git_config_sequence() does (which\n + read_protected_config() is modeled after). Without these NULL checks,\n + multiple tests fail with \"make SANITIZE=address\", e.g. in the final test\n + of t4010, xdg_config is NULL causing us to call fopen(NULL).\n \n Reported-by: Ævar Arnfjörð Bjarmason \n Signed-off-by: Glen Choo \n \n ## config.c ##\n +@@ config.c: int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n + \tint ret = -1;\n + \tFILE *f;\n + \n ++\tif (!filename)\n ++\t\tBUG(\"filename cannot be NULL\");\n + \tf = fopen_or_warn(filename, \"r\");\n + \tif (f) {\n + \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,\n @@ config.c: static void read_protected_config(void)\n \tsystem_config = git_system_config();\n \tgit_global_config(&user_config, &xdg_config);\n\n\n config.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 015bec360f5..e8ebef77d5c 100644\n--- a/config.c\n+++ b/config.c\n@@ -1979,6 +1979,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n \tint ret = -1;\n \tFILE *f;\n \n+\tif (!filename)\n+\t\tBUG(\"filename cannot be NULL\");\n \tf = fopen_or_warn(filename, \"r\");\n \tif (f) {\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,\n@@ -2645,9 +2647,12 @@ static void read_protected_config(void)\n \tsystem_config = git_system_config();\n \tgit_global_config(&user_config, &xdg_config);\n \n-\tgit_configset_add_file(&protected_config, system_config);\n-\tgit_configset_add_file(&protected_config, xdg_config);\n-\tgit_configset_add_file(&protected_config, user_config);\n+\tif (system_config)\n+\t\tgit_configset_add_file(&protected_config, system_config);\n+\tif (xdg_config)\n+\t\tgit_configset_add_file(&protected_config, xdg_config);\n+\tif (user_config)\n+\t\tgit_configset_add_file(&protected_config, user_config);\n \tgit_configset_add_parameters(&protected_config);\n \n \tfree(system_config);\n\nbase-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n-- \ngitgitgadget\n"},{"id":"460012","messageId":"220727.86zgguly2m.gmgdl@evledraar.gmail.com","threadId":"58227","inReplyTo":"kl6lfsinskzm.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] config.c: NULL check when reading protected config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-27T09:08:36Z","receivedAt":"2022-07-27T09:14:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 26 2022, Glen Choo wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:\n>>\n>>> From: Glen Choo \n>>>\n>>> In read_protected_config(), check whether each file name is NULL before\n>>> attempting to read it. This mirrors do_git_config_sequence() (which\n>>> read_protected_config() is modelled after).\n>>>\n>>> Without these NULL checks,\n>>>\n>>> make SANITIZE=address test T=t0410*.sh\n>>>\n>>> fails because xdg_config is NULL, causing us to call fopen(NULL).\n>>\n>> FWIW a lot more than that fails, that's just the test I focused on for\n>> the bug report, the others ones (I didn't check out all of them) all\n>> variants of that.\n>>\n>> See https://github.com/avar/git/runs/7519070124?check_suite_focus=true\n>> for the current failing run with that \"[2]\" patch you quoted. We fail a\n>> total of 14 test files (and many more tests within those files).\n>\n> Ah thanks, I'll amend the message accordingly.\n>\n>>> Reported-by: Ævar Arnfjörð Bjarmason \n>>> Signed-off-by: Glen Choo \n>>> ---\n>>> config.c: NULL check when reading protected config\n>>> \n>>> This fixes the SANITIZE=address failure on master, That was introduced\n>>> by gc/bare-repo-discovery. Thanks again to Ævar for the original report\n>>> [1] and for proposing a way to catch this in CI [2].\n>>> \n>>> [1]\n>>> https://lore.kernel.org/git/220725.861qu9oxl4.gmgdl@evledraar.gmail.com\n>>> [2]\n>>> https://lore.kernel.org/git/patch-1.1-e48b6853dd5-20220726T110716Z-avarab@gmail.com\n>>>\n>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1299%2Fchooglen%2Fconfig%2Ffix-sanitize-address-v1\n>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1299/chooglen/config/fix-sanitize-address-v1\n>>> Pull-Request: https://github.com/git/git/pull/1299\n>>>\n>>> config.c | 9 ++++++---\n>>> 1 file changed, 6 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/config.c b/config.c\n>>> index 015bec360f5..b0ba7f439a4 100644\n>>> --- a/config.c\n>>> +++ b/config.c\n>>> @@ -2645,9 +2645,12 @@ static void read_protected_config(void)\n>>> \tsystem_config = git_system_config();\n>>> \tgit_global_config(&user_config, &xdg_config);\n>>> \n>>> -\tgit_configset_add_file(&protected_config, system_config);\n>>> -\tgit_configset_add_file(&protected_config, xdg_config);\n>>> -\tgit_configset_add_file(&protected_config, user_config);\n>>> +\tif (system_config)\n>>> +\t\tgit_configset_add_file(&protected_config, system_config);\n>>> +\tif (xdg_config)\n>>> +\t\tgit_configset_add_file(&protected_config, xdg_config);\n>>> +\tif (user_config)\n>>> +\t\tgit_configset_add_file(&protected_config, user_config);\n>>> \tgit_configset_add_parameters(&protected_config);\n>>> \n>>> \tfree(system_config);\n>>>\n>>> base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\n>>\n>> Re your claim in\n>> https://lore.kernel.org/git/kl6lzggwsyh1.fsf@chooglen-macbookpro.roam.corp.google.com/\n>> I tried testing this, and came up with the below.\n>>\n>> I wonder if we should work in here for general paranoia, but I'm not too\n>> familiar with the this part of the config reading, maybe we're confident\n>> enough that these are invariants within the process.\n>>\n>> This will BUG() out if these variables change within the process, which\n>> would mean that our caching assumptions are no longer true, which would\n>> cause you to return the wrong data here.\n>>\n>> Of course you'd have segfaulted or similar before, but this should\n>> demonstrate that not only are these sometimes NULL, but that they stay\n>> that way.\n>\n> Interesting, this is worth proposing, but I suspect that the\n> conversation will be long enough for this to be its own thread. Surely\n> someone must have given some thought to this, especially for long-lived\n> processes (git-daemon?).\n>\n> There's also the general question of config cache freshness, e.g. what\n> if another git process writes to a shared config file? (We don't worry\n> about the single process case because the process will invalidate its\n> own cache).\n>\n> Perhaps we should also worry about that (probably more common) case in\n> addition to this one? At any rate, that seems like a bigger topic than\n> this fix here.\n\nWe can leave it for later, I've run it as a one-off and didn't have any\nfailures.\n\nBut FWIW I think it's tied up in this fix here, i.e. your original code\nboth added caching, and implicitly assumed that these were never NULL,\nso it was \"obvious\" that it didn't need such assertions.\n\nBut now we have 3x if's in a code path that's cached, and the cache is\n*not* guarded by the same 3x checks.\n\nSo we can leave it for later, but it really seems worth adding some\nself-documentation here sooner than later.\n\nThis BUG() method I came up with is one way, another would be to strdup\nit and use a \"static\" variable in the function, i.e. stick with whatever\nvalue(s) we start out with.\n\nBut in any case, this fix seems correct, and fixes the current issues\nSANITIZE=address is spotting for us, thanks!\n"},{"id":"460013","messageId":"220727.86v8rilxx1.gmgdl@evledraar.gmail.com","threadId":"58227","inReplyTo":"pull.1299.v2.git.git.1658874067077.gitgitgadget@gmail.com","subject":"nonnull v.s. BUG() if !x (was: [PATCH v2] config.c: NULL check when reading protected config)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-27T09:12:22Z","receivedAt":"2022-07-27T09:15:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n\n> +\tif (!filename)\n> +\t\tBUG(\"filename cannot be NULL\");\n\nLooks good, but as an aside I wonder if we wouldn't get better code\nanalysis with \"nonnull\" for this sort of thing, but we can leave this\nfor now:\nhttps://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes\n"},{"id":"460027","messageId":"xmqq1qu6txb0.fsf@gitster.g","threadId":"58227","inReplyTo":"pull.1299.v2.git.git.1658874067077.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] config.c: NULL check when reading protected config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-27T15:00:51Z","receivedAt":"2022-07-27T15:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" writes:\n\n> diff --git a/config.c b/config.c\n> index 015bec360f5..e8ebef77d5c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2645,9 +2647,12 @@ static void read_protected_config(void)\n> \tsystem_config = git_system_config();\n> \tgit_global_config(&user_config, &xdg_config);\n> \n> -\tgit_configset_add_file(&protected_config, system_config);\n> -\tgit_configset_add_file(&protected_config, xdg_config);\n> -\tgit_configset_add_file(&protected_config, user_config);\n> +\tif (system_config)\n> +\t\tgit_configset_add_file(&protected_config, system_config);\n> +\tif (xdg_config)\n> +\t\tgit_configset_add_file(&protected_config, xdg_config);\n> +\tif (user_config)\n> +\t\tgit_configset_add_file(&protected_config, user_config);\n> \tgit_configset_add_parameters(&protected_config);\n\nThis does make it similar to the way how the primary \"config\nsequence\" reader calls git_config_from_file(), so I do prefer the\npatch as posted as a short-term \"oops, we merged a buggy code that\nsegfaults and here is a fix\" patch.\n\nIt however makes me wonder if it is simpler to allow passing NULL to\ngit_config_from_file_with_options() and make it silently turn into a\nno-op. I.e. instead of ...\n\n> @@ -1979,6 +1979,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n> \tint ret = -1;\n> \tFILE *f;\n> \n> +\tif (!filename)\n> +\t\tBUG(\"filename cannot be NULL\");\n\n... we could do\n\n\tif (!filename)\n\t\treturn 0; /* successful no-op */\n\nEven if there are codepaths that feed arbitrary pathnames given by\nthe end user, they wouldn't be passing NULL (they may pass an empty\nstring, or a filename that causes fopen() to fail), would they?\n\nBut that is something we should leave to a follow-up series, not\n\"oops, we need to fix it now\" fix.\n\nThanks, will queue.\n\n> \tf = fopen_or_warn(filename, \"r\");\n> \tif (f) {\n> \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,\n"},{"id":"460029","messageId":"kl6lczdqsdkz.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58227","inReplyTo":"xmqq1qu6txb0.fsf@gitster.g","subject":"Re: [PATCH v2] config.c: NULL check when reading protected config","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-27T16:52:12Z","receivedAt":"2022-07-27T17:43:16Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> It however makes me wonder if it is simpler to allow passing NULL to\n> git_config_from_file_with_options() and make it silently turn into a\n> no-op. I.e. instead of ...\n>\n>> @@ -1979,6 +1979,8 @@ int git_config_from_file_with_options(config_fn_t fn, const char *filename,\n>> \tint ret = -1;\n>> \tFILE *f;\n>> \n>> +\tif (!filename)\n>> +\t\tBUG(\"filename cannot be NULL\");\n>\n> ... we could do\n>\n> \tif (!filename)\n> \t\treturn 0; /* successful no-op */\n>\n> Even if there are codepaths that feed arbitrary pathnames given by\n> the end user, they wouldn't be passing NULL (they may pass an empty\n> string, or a filename that causes fopen() to fail), would they?\n\nYeah, that's worth considering. I'm not sure how I feel about it yet,\nbut hopefully I find some time to dig around and form an opinion.\n\n>\n> But that is something we should leave to a follow-up series, not\n> \"oops, we need to fix it now\" fix.\n>\n> Thanks, will queue.\n\nThanks :)\n\n>\n>> \tf = fopen_or_warn(filename, \"r\");\n>> \tif (f) {\n>> \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename,\n"},{"id":"460030","messageId":"kl6la68uscw0.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58227","inReplyTo":"220727.86v8rilxx1.gmgdl@evledraar.gmail.com","subject":"Re: nonnull v.s. BUG() if !x (was: [PATCH v2] config.c: NULL check when reading protected config)","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-07-27T17:07:11Z","receivedAt":"2022-07-27T18:03:40Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, Jul 26 2022, Glen Choo via GitGitGadget wrote:\n>\n>> From: Glen Choo \n>\n>> +\tif (!filename)\n>> +\t\tBUG(\"filename cannot be NULL\");\n>\n> Looks good, but as an aside I wonder if we wouldn't get better code\n> analysis with \"nonnull\" for this sort of thing, but we can leave this\n> for now:\n> https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes\n\nInteresting. I wonder how good the analysis is vs the cost, e.g. it's\nuseful if it detects _maybe_ NULL variables, but it might be too\nexpensive if it requires us to mark all of our variables as non-NULL.\n"}]}