| Message ID | da1c23a9ccf8c797ebcbe6ce5a8243c1d051fad6.1724656120.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.6) | expand |
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. 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
On Wed, Sep 4, 2024 at 3:56 PM Junio C Hamano <gitster@pobox.com> wrote: > > 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`. 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@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>
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
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. So let's fix that, which will also plug the memory leak for us. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- merge-ort.c | 4 +++- t/t6423-merge-rename-directories.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)