| Message ID | 2a63030ff09f938d705c117406b501ecf81f67de.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: > We do not free several struct members in `clear_shallow_info()`. Fix > this to plug the resulting leaks. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > shallow.c | 9 +++++++++ > t/t5538-push-shallow.sh | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/shallow.c b/shallow.c > index 7e0ee96ead9..dcebc263d70 100644 > --- a/shallow.c > +++ b/shallow.c > @@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa) > > void clear_shallow_info(struct shallow_info *info) > { > + if (info->used_shallow) { > + for (size_t i = 0; i < info->shallow->nr; i++) > + free(info->used_shallow[i]); > + free(info->used_shallow); > + } > + > + free(info->need_reachability_test); > + free(info->reachable); > + free(info->shallow_ref); > free(info->ours); > free(info->theirs); > } Recently was agreed in the CodingGuidelines `S_clear()` functions do a `S_release()` + `S_init()`. I see we're not initializing the struct for future use (i.e. we don't reset the `nr_*` fields to 0). But we cannot really do an init, because that would be calling `prepare_shallow_info()`, which allocates new memory. So would it be worth to rename this function to `release_shallow_info()`? -- Toon
Toon claes <toon@iotcl.com> writes: >> void clear_shallow_info(struct shallow_info *info) >> { >> + if (info->used_shallow) { >> + for (size_t i = 0; i < info->shallow->nr; i++) >> + free(info->used_shallow[i]); >> + free(info->used_shallow); >> + } >> + >> + free(info->need_reachability_test); >> + free(info->reachable); >> + free(info->shallow_ref); >> free(info->ours); >> free(info->theirs); >> } > > `prepare_shallow_info()`, which allocates new memory. So would it be > worth to rename this function to `release_shallow_info()`? In the longer term in a separate "renaming everything" effort, yes. In the context of "plug many resource leaks" series, probably no. Thanks.
On Thu, Aug 29, 2024 at 09:07:47AM -0700, Junio C Hamano wrote: > Toon claes <toon@iotcl.com> writes: > > >> void clear_shallow_info(struct shallow_info *info) > >> { > >> + if (info->used_shallow) { > >> + for (size_t i = 0; i < info->shallow->nr; i++) > >> + free(info->used_shallow[i]); > >> + free(info->used_shallow); > >> + } > >> + > >> + free(info->need_reachability_test); > >> + free(info->reachable); > >> + free(info->shallow_ref); > >> free(info->ours); > >> free(info->theirs); > >> } > > > > `prepare_shallow_info()`, which allocates new memory. So would it be > > worth to rename this function to `release_shallow_info()`? > > In the longer term in a separate "renaming everything" effort, yes. > In the context of "plug many resource leaks" series, probably no. Yeah, agreed. And note that `release_shallow_info()` does not match our style guide, either: it should be `shallow_info_release()`. Patrick
diff --git a/shallow.c b/shallow.c index 7e0ee96ead9..dcebc263d70 100644 --- a/shallow.c +++ b/shallow.c @@ -489,6 +489,15 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa) void clear_shallow_info(struct shallow_info *info) { + if (info->used_shallow) { + for (size_t i = 0; i < info->shallow->nr; i++) + free(info->used_shallow[i]); + free(info->used_shallow); + } + + free(info->need_reachability_test); + free(info->reachable); + free(info->shallow_ref); free(info->ours); free(info->theirs); } diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh index e91fcc173e8..6adc3a20a45 100755 --- a/t/t5538-push-shallow.sh +++ b/t/t5538-push-shallow.sh @@ -5,6 +5,7 @@ test_description='push from/to a shallow clone' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh commit() {
We do not free several struct members in `clear_shallow_info()`. Fix this to plug the resulting leaks. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- shallow.c | 9 +++++++++ t/t5538-push-shallow.sh | 1 + 2 files changed, 10 insertions(+)