Re: [PATCH 21/22] merge-ort: fix two leaks when handling directory rename modifications

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux