Message ID | patch-01.14-bcba06e1d28-20220302T170718Z-avarab@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | tree-wide: small fixes for memory leaks | expand |
On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote: > Fix various memory leaks in "git index-pack", due to how tightly > coupled this command is with the revision walking this doesn't make > any new tests pass, but e.g. this now passes, and had several failures before: > > ./t5300-pack-object.sh --run=1-2,4,6-27,30-42 Do you mean that these tests now pass under leak check? > it is a bit odd that we'll free "opts.anomaly", since the "opts" is a s/it/It/ > "struct pack_idx_option" declared in pack.h. In pack-write.c there's a > reset_pack_idx_option(), but it only wipes the contents, but doesn't > free() anything. > > Doing this here in cmd_index_pack() is correct because while the > struct is declared in pack.h, this code in builtin/index-pack.c (in > read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so > we should also free it here. Makes sense. Code diff looks good. Thanks, -Stolee
On Wed, Mar 02 2022, Derrick Stolee wrote: > On 3/2/2022 12:10 PM, Ævar Arnfjörð Bjarmason wrote: >> Fix various memory leaks in "git index-pack", due to how tightly >> coupled this command is with the revision walking this doesn't make >> any new tests pass, but e.g. this now passes, and had several failures before: >> >> ./t5300-pack-object.sh --run=1-2,4,6-27,30-42 > > Do you mean that these tests now pass under leak check? Yes, i.e. test 3, 5 etc. fails before & after, and this change makes tests in these ranges pass. >> it is a bit odd that we'll free "opts.anomaly", since the "opts" is a > > s/it/It/ Thanks, will fix! >> "struct pack_idx_option" declared in pack.h. In pack-write.c there's a >> reset_pack_idx_option(), but it only wipes the contents, but doesn't >> free() anything. >> >> Doing this here in cmd_index_pack() is correct because while the >> struct is declared in pack.h, this code in builtin/index-pack.c (in >> read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so >> we should also free it here. > > Makes sense. Code diff looks good. Thanks!
diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 3c2e6aee3cc..5fe1adb3681 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1109,6 +1109,7 @@ static void *threaded_second_pass(void *data) list_add(&child->list, &work_head); base_cache_used += child->size; prune_base_data(NULL); + free_base_data(child); } else { /* * This child does not have its own children. It may be @@ -1131,6 +1132,7 @@ static void *threaded_second_pass(void *data) p = next_p; } + FREE_AND_NULL(child); } work_unlock(); } @@ -1424,6 +1426,7 @@ static void fix_unresolved_deltas(struct hashfile *f) * object). */ append_obj_to_pack(f, d->oid.hash, data, size, type); + free(data); threaded_second_pass(NULL); display_progress(progress, nr_resolved_deltas); @@ -1703,6 +1706,7 @@ static void show_pack_info(int stat_only) i + 1, chain_histogram[i]); } + free(chain_histogram); } int cmd_index_pack(int argc, const char **argv, const char *prefix) @@ -1932,6 +1936,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) if (do_fsck_object && fsck_finish(&fsck_options)) die(_("fsck error in pack objects")); + free(opts.anomaly); free(objects); strbuf_release(&index_name_buf); strbuf_release(&rev_index_name_buf);
Fix various memory leaks in "git index-pack", due to how tightly coupled this command is with the revision walking this doesn't make any new tests pass, but e.g. this now passes, and had several failures before: ./t5300-pack-object.sh --run=1-2,4,6-27,30-42 it is a bit odd that we'll free "opts.anomaly", since the "opts" is a "struct pack_idx_option" declared in pack.h. In pack-write.c there's a reset_pack_idx_option(), but it only wipes the contents, but doesn't free() anything. Doing this here in cmd_index_pack() is correct because while the struct is declared in pack.h, this code in builtin/index-pack.c (in read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so we should also free it here. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/index-pack.c | 5 +++++ 1 file changed, 5 insertions(+)