| Message ID | 101d6a2841a909aa9c99225ebf1b65695cf116cb.1701198172.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-objects: multi-pack verbatim reuse | expand |
On Tue, Nov 28, 2023 at 02:07:57PM -0500, Taylor Blau wrote: > The pack-objects internals use a packing_data struct to track what > objects are part of the pack(s) being formed. > > Since these structures contain allocated fields, failing to > appropriately free() them results in a leak. Plug that leak by > introducing a free_packing_data() function, and call it in the > appropriate spots. > > This is a fairly straightforward leak to plug, since none of the callers > expect to read any values or have any references to parts of the address > space being freed. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > builtin/pack-objects.c | 1 + > midx.c | 5 +++++ > pack-objects.c | 15 +++++++++++++++ > pack-objects.h | 1 + > 4 files changed, 22 insertions(+) > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 89a8b5a976..bfa60359d4 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -4522,6 +4522,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) > reuse_packfile_objects); > > cleanup: > + free_packing_data(&to_pack); > list_objects_filter_release(&filter_options); > strvec_clear(&rp); > > diff --git a/midx.c b/midx.c > index 2f3863c936..3b727dc633 100644 > --- a/midx.c > +++ b/midx.c > @@ -1592,8 +1592,13 @@ static int write_midx_internal(const char *object_dir, > flags) < 0) { > error(_("could not write multi-pack bitmap")); > result = 1; > + free_packing_data(&pdata); > + free(commits); > goto cleanup; > } > + > + free_packing_data(&pdata); > + free(commits); > } > /* > * NOTE: Do not use ctx.entries beyond this point, since it might > diff --git a/pack-objects.c b/pack-objects.c > index f403ca6986..1c7bedcc94 100644 > --- a/pack-objects.c > +++ b/pack-objects.c > @@ -151,6 +151,21 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata) > init_recursive_mutex(&pdata->odb_lock); > } > > +void free_packing_data(struct packing_data *pdata) Nit: shouldn't this rather be called `clear_packing_data`? `free` to me indicates that the data structure itself will be free'd, as well, which is not the case. Patrick > +{ > + if (!pdata) > + return; > + > + free(pdata->cruft_mtime); > + free(pdata->in_pack); > + free(pdata->in_pack_by_idx); > + free(pdata->in_pack_pos); > + free(pdata->index); > + free(pdata->layer); > + free(pdata->objects); > + free(pdata->tree_depth); > +} > + > struct object_entry *packlist_alloc(struct packing_data *pdata, > const struct object_id *oid) > { > diff --git a/pack-objects.h b/pack-objects.h > index 0d78db40cb..336217e8cd 100644 > --- a/pack-objects.h > +++ b/pack-objects.h > @@ -169,6 +169,7 @@ struct packing_data { > }; > > void prepare_packing_data(struct repository *r, struct packing_data *pdata); > +void free_packing_data(struct packing_data *pdata); > > /* Protect access to object database */ > static inline void packing_data_lock(struct packing_data *pdata) > -- > 2.43.0.24.g980b318f98 >
On Thu, Nov 30, 2023 at 11:18:26AM +0100, Patrick Steinhardt wrote: > > diff --git a/pack-objects.c b/pack-objects.c > > index f403ca6986..1c7bedcc94 100644 > > --- a/pack-objects.c > > +++ b/pack-objects.c > > @@ -151,6 +151,21 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata) > > init_recursive_mutex(&pdata->odb_lock); > > } > > > > +void free_packing_data(struct packing_data *pdata) > > Nit: shouldn't this rather be called `clear_packing_data`? `free` to me > indicates that the data structure itself will be free'd, as well, which > is not the case. Thanks, that's a good suggestion. I've made the changes locally and will include it in the subsequent round(s) ;-). Thanks, Taylor
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 89a8b5a976..bfa60359d4 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4522,6 +4522,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) reuse_packfile_objects); cleanup: + free_packing_data(&to_pack); list_objects_filter_release(&filter_options); strvec_clear(&rp); diff --git a/midx.c b/midx.c index 2f3863c936..3b727dc633 100644 --- a/midx.c +++ b/midx.c @@ -1592,8 +1592,13 @@ static int write_midx_internal(const char *object_dir, flags) < 0) { error(_("could not write multi-pack bitmap")); result = 1; + free_packing_data(&pdata); + free(commits); goto cleanup; } + + free_packing_data(&pdata); + free(commits); } /* * NOTE: Do not use ctx.entries beyond this point, since it might diff --git a/pack-objects.c b/pack-objects.c index f403ca6986..1c7bedcc94 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -151,6 +151,21 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata) init_recursive_mutex(&pdata->odb_lock); } +void free_packing_data(struct packing_data *pdata) +{ + if (!pdata) + return; + + free(pdata->cruft_mtime); + free(pdata->in_pack); + free(pdata->in_pack_by_idx); + free(pdata->in_pack_pos); + free(pdata->index); + free(pdata->layer); + free(pdata->objects); + free(pdata->tree_depth); +} + struct object_entry *packlist_alloc(struct packing_data *pdata, const struct object_id *oid) { diff --git a/pack-objects.h b/pack-objects.h index 0d78db40cb..336217e8cd 100644 --- a/pack-objects.h +++ b/pack-objects.h @@ -169,6 +169,7 @@ struct packing_data { }; void prepare_packing_data(struct repository *r, struct packing_data *pdata); +void free_packing_data(struct packing_data *pdata); /* Protect access to object database */ static inline void packing_data_lock(struct packing_data *pdata)
The pack-objects internals use a packing_data struct to track what objects are part of the pack(s) being formed. Since these structures contain allocated fields, failing to appropriately free() them results in a leak. Plug that leak by introducing a free_packing_data() function, and call it in the appropriate spots. This is a fairly straightforward leak to plug, since none of the callers expect to read any values or have any references to parts of the address space being freed. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/pack-objects.c | 1 + midx.c | 5 +++++ pack-objects.c | 15 +++++++++++++++ pack-objects.h | 1 + 4 files changed, 22 insertions(+)