From: Glen Choo Date: Tue, 26 Jul 2022 01:06:37 GMT Subject: Re: [PATCH v3 24/26] submodule--helper: free rest of "displaypath" in "struct update_data" Message-ID: In-Reply-To: Ævar Arnfjörð Bjarmason writes: > Fix a leak in code added in c51f8f94e5b (submodule--helper: run update > procedures from C, 2021-08-24), we clobber the "displaypath" member of > the passed-in "struct update_data" both so that die() messages in this > update_submodule() function itself can use it, and for the > run_update_procedure() called within this function. > > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > builtin/submodule--helper.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index b65665105e7..4e70a74357c 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2551,10 +2551,11 @@ static int update_submodule(struct update_data *update_data, > int *must_die_on_failure) > { > int ret = 1; > + char *to_free, *restore = update_data->displaypath; > > ensure_core_worktree(update_data->sm_path); > > - update_data->displaypath = get_submodule_displaypath( > + update_data->displaypath = to_free = get_submodule_displaypath( > update_data->sm_path, update_data->prefix); > > determine_submodule_update_strategy(the_repository, update_data->just_cloned, > @@ -2628,6 +2629,9 @@ static int update_submodule(struct update_data *update_data, > > ret = 0; > cleanup: > + free(to_free); > + update_data->displaypath = restore; > + > return ret; > } I'm not sure why we need to have "restore". We set "update_data->displaypath" so that we can use it inside this function (and its call chain), but we don't care about it outside of this call chain at all. If the goal is to avoid exposing a free()-d pointer, could we just do FREE_AND_NULL(update_data->displaypath); instead? > > -- > 2.37.1.1095.g0bd6f54ba8a