Re: [PATCH v2] config.c: NULL check when reading protected config
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 27, 2022, 15:00 UTC
- Message-ID
- <xmqq1qu6txb0.fsf@gitster.g>
- In-Reply-To
- <pull.1299.v2.git.git.1658874067077.gitgitgadget@gmail.com>
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 18 quoted lines
> 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 ...
Show 6 quoted lines
> @@ -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,