Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications
On Wed, Sep 4, 2024 at 3:56 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
>
> Patrick Steinhardt <ps@pks.im> 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`.
> > 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.
Show 45 quoted lines
> > So let's fix that, which will also plug the memory leak for us.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
>
> 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@gmail.com>
Elijah Newren <newren@gmail.com>