On Tue, Jul 26, 2022 at 05:09:32PM +0000, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@xxxxxxxxxx> > > 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
