On Wed, Sep 4, 2024 at 3:56 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Patrick Steinhardt <ps@xxxxxx> writes: > > > There are two leaks in `apply_directory_rename_modifications()`: > > > > - We do not release the `dirs_to_insert` string list. > > > > - We do not release some `conflict_info` we put into the > > `opt->priv->paths` string map. > > > > The former is trivial to fix. The latter is a bit less straight forward: > > the `util` pointer of the string map may sometimes point to data that > > has been allocated via `CALLOC()`, while at other times it may point to > > data that has been allocated via a `mem_pool`. Oops. > > It very much seems like an oversight that we didn't also allocate the > > conflict info in this code path via the memory pool, though. Yep. > > So let's fix that, which will also plug the memory leak for us. > > > > Signed-off-by: Patrick Steinhardt <ps@xxxxxx> > > --- > > May be nice if we can hear from the original author and the area > expert. > > > merge-ort.c | 4 +++- > > t/t6423-merge-rename-directories.sh | 1 + > > 2 files changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/merge-ort.c b/merge-ort.c > > index 3752c7e595d..0ed3cd06b1a 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -2710,7 +2710,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt, > > struct conflict_info *dir_ci; > > char *cur_dir = dirs_to_insert.items[i].string; > > > > - CALLOC_ARRAY(dir_ci, 1); > > + dir_ci = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*dir_ci)); > > > > dir_ci->merged.directory_name = parent_name; > > len = strlen(parent_name); > > @@ -2838,6 +2838,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt, > > * Finally, record the new location. > > */ > > pair->two->path = new_path; > > + > > + string_list_clear(&dirs_to_insert, 0); > > } > > > > /*** Function Grouping: functions related to regular rename detection ***/ > > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > > index 88d1cf2cde9..4aaaf38be68 100755 > > --- a/t/t6423-merge-rename-directories.sh > > +++ b/t/t6423-merge-rename-directories.sh > > @@ -25,6 +25,7 @@ test_description="recursive merge with directory renames" > > # underscore notation is to differentiate different > > # files that might be renamed into each other's paths.) > > > > +TEST_PASSES_SANITIZE_LEAK=true > > . ./test-lib.sh > > . "$TEST_DIRECTORY"/lib-merge.sh Looks good to me; thanks for fixing up my bugs. Reviewed-by: Elijah Newren <newren@xxxxxxxxx>
