Taylor Blau <me@xxxxxxxxxxxx> writes: > 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 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
