| Message ID | patch-09.11-7b36f71879e-20220713T131601Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule--helper: fix memory leaks | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Make the update_data_release() function free the "recursive_prefix" > and "displaypath" members when appropriate. For the former it could > come from either "argv" or from our own allocation, so we need to keep > track of a "to_free" sibling seperately. Obsolete message probably? "recursive_prefix" no longer exists as of gc/submodule-use-super-prefix ;) > For "displaypath" it's always ours, so the "const char *" type was > wrong to begin with, it should be a "char *" instead. Ok. > For update_submodule() we'll free() these as we go along, it's called > in a loop by update_submodules(), and we'll need to free the "last" > one. Hm, I don't follow this part. Does "as we go along" mean "as we go along freeing things in update_submodules()", or "we'll do this later on"? I'm assuming it's the latter since this patch only frees the "last" one and doesn't free inside of update_submodule(), but maybe it's not so hard to do? I think it's just: diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 0bac39880d..34b54e97d1 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2560,6 +2560,7 @@ static int update_submodule(struct update_data *update_data) { ensure_core_worktree(update_data->sm_path); + free(update_data->displaypath); update_data->displaypath = get_submodule_displaypath( update_data->sm_path, update_data->prefix); > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/submodule--helper.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 2b44f391f15..0bac39880d2 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1949,7 +1949,7 @@ static void submodule_update_clone_release(struct submodule_update_clone *suc) > > struct update_data { > const char *prefix; > - const char *displaypath; > + char *displaypath; > enum submodule_update_type update_default; > struct object_id suboid; > struct string_list references; > @@ -1987,6 +1987,7 @@ struct update_data { > > static void update_data_release(struct update_data *ud) > { > + free(ud->displaypath); > module_list_release(&ud->list); > } > > -- > 2.37.0.932.g7b7031e73bc
On Thu, Jul 14 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Make the update_data_release() function free the "recursive_prefix" >> and "displaypath" members when appropriate. For the former it could >> come from either "argv" or from our own allocation, so we need to keep >> track of a "to_free" sibling seperately. > > Obsolete message probably? "recursive_prefix" no longer exists as of > gc/submodule-use-super-prefix ;) Thanks, will fix! >> For "displaypath" it's always ours, so the "const char *" type was >> wrong to begin with, it should be a "char *" instead. > > Ok. > >> For update_submodule() we'll free() these as we go along, it's called >> in a loop by update_submodules(), and we'll need to free the "last" >> one. > > Hm, I don't follow this part. Does "as we go along" mean "as we go along > freeing things in update_submodules()", or "we'll do this later on"? You know what? After looking at this again I have no idea what I was talking about here, and I think this makes no sense... :) > I'm assuming it's the latter since this patch only frees the "last" one > and doesn't free inside of update_submodule(), but maybe it's not so > hard to do? I think it's just: > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 0bac39880d..34b54e97d1 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2560,6 +2560,7 @@ static int update_submodule(struct update_data *update_data) > { > ensure_core_worktree(update_data->sm_path); > > + free(update_data->displaypath); > update_data->displaypath = get_submodule_displaypath( > update_data->sm_path, update_data->prefix); Thanks, I'll fix this case in a re-roll, fixing it generally turned out to be a lot trickier though, but in the process I fixed a lot of the control flow issues. Am currently running extensive tests on it. Thanks a lot for this & other reviews, it's been really useful, as always.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 2b44f391f15..0bac39880d2 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1949,7 +1949,7 @@ static void submodule_update_clone_release(struct submodule_update_clone *suc) struct update_data { const char *prefix; - const char *displaypath; + char *displaypath; enum submodule_update_type update_default; struct object_id suboid; struct string_list references; @@ -1987,6 +1987,7 @@ struct update_data { static void update_data_release(struct update_data *ud) { + free(ud->displaypath); module_list_release(&ud->list); }
Make the update_data_release() function free the "recursive_prefix" and "displaypath" members when appropriate. For the former it could come from either "argv" or from our own allocation, so we need to keep track of a "to_free" sibling seperately. For "displaypath" it's always ours, so the "const char *" type was wrong to begin with, it should be a "char *" instead. For update_submodule() we'll free() these as we go along, it's called in a loop by update_submodules(), and we'll need to free the "last" one. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)