| Message ID | 9591fb7b5e1dac2f989bd10ef2c13a191571a060.1723540931.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
Patrick Steinhardt <ps@pks.im> writes: > Before calling `handle_commit()` in a loop, we set `diffopt.no_free` > such that its contents aren't getting freed inside of `handle_commit()`. > We never unset that flag though, which means that it'll ultimately leak > when calling `release_revisions()`. > > Fix this by unsetting the flag after the loop. If I grep for $ git grep -nH -E -e '(\.|->)no_free' \*.c I notice that in a lot of places there is a pattern of doing set .no_free to 1 cause a bunch of diff using the same set of options set .no_free to 0 call diff_free(). I am curious why we do not need any diff_free() here? > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/fast-export.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/builtin/fast-export.c b/builtin/fast-export.c > index 4b6e8c6832..fe92d2436c 100644 > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > @@ -1278,9 +1278,11 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) > revs.diffopt.format_callback = show_filemodify; > revs.diffopt.format_callback_data = &paths_of_changed_objects; > revs.diffopt.flags.recursive = 1; > + > revs.diffopt.no_free = 1; > while ((commit = get_revision(&revs))) > handle_commit(commit, &revs, &paths_of_changed_objects); > + revs.diffopt.no_free = 0; > > handle_tags_and_duplicates(&extra_refs); > handle_tags_and_duplicates(&tag_refs);
On Tue, Aug 13, 2024 at 09:34:40AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Before calling `handle_commit()` in a loop, we set `diffopt.no_free` > > such that its contents aren't getting freed inside of `handle_commit()`. > > We never unset that flag though, which means that it'll ultimately leak > > when calling `release_revisions()`. > > > > Fix this by unsetting the flag after the loop. > > If I grep for > > $ git grep -nH -E -e '(\.|->)no_free' \*.c > > I notice that in a lot of places there is a pattern of doing > > set .no_free to 1 > cause a bunch of diff using the same set of options > set .no_free to 0 > call diff_free(). > > I am curious why we do not need any diff_free() here? Because it's already being called via `release_revisions()`. Patrick
diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 4b6e8c6832..fe92d2436c 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -1278,9 +1278,11 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) revs.diffopt.format_callback = show_filemodify; revs.diffopt.format_callback_data = &paths_of_changed_objects; revs.diffopt.flags.recursive = 1; + revs.diffopt.no_free = 1; while ((commit = get_revision(&revs))) handle_commit(commit, &revs, &paths_of_changed_objects); + revs.diffopt.no_free = 0; handle_tags_and_duplicates(&extra_refs); handle_tags_and_duplicates(&tag_refs);
Before calling `handle_commit()` in a loop, we set `diffopt.no_free` such that its contents aren't getting freed inside of `handle_commit()`. We never unset that flag though, which means that it'll ultimately leak when calling `release_revisions()`. Fix this by unsetting the flag after the loop. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fast-export.c | 2 ++ 1 file changed, 2 insertions(+)