| Message ID | pull.1299.v2.git.git.1658874067077.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 776f184893d2861a729aa4b91d69931036e03e4b |
| Headers | show |
| Series | [v2] config.c: NULL check when reading protected config | expand |
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
"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,
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,
Æ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.
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);