Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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. > > To make managing that clobbering easier let's wrap the > update_submodule() in a new update_submodule_outer() function, which > will do the clobbering and free(to_free) dance for us. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > --- > builtin/submodule--helper.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 79eca6c439b..cc8f42ae6df 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2482,13 +2482,6 @@ static int update_submodule(struct update_data *update_data) > { > int ret; > > - ret = ensure_core_worktree(update_data->sm_path); > - if (ret) > - return ret; > - > - update_data->displaypath = get_submodule_displaypath( > - update_data->sm_path, update_data->prefix); > - > ret = determine_submodule_update_strategy(the_repository, > update_data->just_cloned, > update_data->sm_path, > @@ -2554,6 +2547,24 @@ static int update_submodule(struct update_data *update_data) > return 0; > } > > +static int update_submodule_outer(struct update_data *update_data) > +{ > + char *to_free, *restore = update_data->displaypath; > + int ret; > + > + ensure_core_worktree(update_data->sm_path); > + > + update_data->displaypath = to_free = get_submodule_displaypath( > + update_data->sm_path, update_data->prefix); > + > + ret = update_submodule(update_data); > + > + free(to_free); > + update_data->displaypath = restore; > + > + return ret; > +} > + I haven't tested the alternatives yet, but on first glance using this *_outer() pattern here seems like overkill. At least two things I can think of are: a) Free .displaypath using the "goto cleanup" pattern like we did elsewhere ... > static int update_submodules(struct update_data *update_data) > { > int i, ret = 0; > @@ -2586,7 +2597,7 @@ static int update_submodules(struct update_data *update_data) > update_data->just_cloned = ucd.just_cloned; > update_data->sm_path = ucd.sub->path; > > - code = update_submodule(update_data); > + code = update_submodule_outer(update_data); > if (code) > ret = code; > if (code == 128) b) Assign and FREE_AND_NULL() update_data->displaypath here since this is the only caller and it already does some prep work in this hunk. I started testing, but then realized that we don't have a TEST_PASSES_SANITIZE_LEAK=true for this patch, so I'll spend some more time testing whether these alternatives even work and I'll get back to you later. > -- > 2.37.1.1233.ge8b09efaedc