| Message ID | c048b54a2c493658b2dd256b301491a79cfa99a1.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: > There are various memory leaks hit by git-format-patch(1). Basically all > of them are trivial, except that un-setting `diffopt.no_free` requires > us to unset the `diffopt.file` because we manually close it already. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/log.c | 12 +++++++++--- > t/t4014-format-patch.sh | 1 + > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/builtin/log.c b/builtin/log.c > index a73a767606..ff997a0d0e 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1833,6 +1833,7 @@ static struct commit *get_base_commit(const struct format_config *cfg, > } > > rev[i] = merge_base->item; > + free_commit_list(merge_base); > } > > if (rev_nr % 2) This is correct, but isn't merge_base leaking when there are multiple and we are not dying on failure? Perhaps something along this line? struct commit_list *merge_base = NULL; if (repo_get_merge_bases(the_repository, rev[2 * i], rev[2 * i + 1], &merge_base) < 0 || !merge_base || merge_base->next) { if (die_on_failure) { die(_("failed to find exact merge base")); } else { + free_commit_list(merge_base); free(rev); return NULL; } } > @@ -2548,12 +2550,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > else > print_signature(signature, rev.diffopt.file); > } > - if (output_directory) > + if (output_directory) { > fclose(rev.diffopt.file); > + rev.diffopt.file = NULL; Is this a leakfix, or just a general code hygiene improvement? > + } > } > stop_progress(&progress); > free(list); > - free(branch_name); > if (ignore_if_in_upstream) > free_patch_ids(&ids); Good eyes. branch_name can be set and then "goto done" can jump this one over, so it makes sense to move it below and make it part of the centralized clean-up. list is not leaking in the current code, and there is no "goto done" or "return" after it gets allocated before this point, so it can stay here. On the other hand, it appears to me that everything below stop_progress() we see above can be moved below the "done:" label, except for that ids may still be left uninitialized without getting populated by get_patch_ids() when ignore-if-in-upstream is asked but head and upstream are the same when we jump to the "done:" label, so it needs a bit more work _if_ we wanted to go that route. I think the postimage of this patch, i.e. freeing the "list" and "ids" here before the "done:" label, is a good place to stop. Thanks.
Patrick Steinhardt <ps@pks.im> writes: > There are various memory leaks hit by git-format-patch(1). Basically all > of them are trivial, except that un-setting `diffopt.no_free` requires > us to unset the `diffopt.file` because we manually close it already. Ah, I misread the patch. Clearly diffopt.file is about double-closing. Makes sense.
On Tue, Aug 13, 2024 at 09:55:10AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > There are various memory leaks hit by git-format-patch(1). Basically all > > of them are trivial, except that un-setting `diffopt.no_free` requires > > us to unset the `diffopt.file` because we manually close it already. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > builtin/log.c | 12 +++++++++--- > > t/t4014-format-patch.sh | 1 + > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/builtin/log.c b/builtin/log.c > > index a73a767606..ff997a0d0e 100644 > > --- a/builtin/log.c > > +++ b/builtin/log.c > > @@ -1833,6 +1833,7 @@ static struct commit *get_base_commit(const struct format_config *cfg, > > } > > > > rev[i] = merge_base->item; > > + free_commit_list(merge_base); > > } > > > > if (rev_nr % 2) > > This is correct, but isn't merge_base leaking when there are > multiple and we are not dying on failure? Perhaps something along > this line? Yes, good catch. > struct commit_list *merge_base = NULL; > if (repo_get_merge_bases(the_repository, > rev[2 * i], > rev[2 * i + 1], &merge_base) < 0 || > !merge_base || merge_base->next) { > if (die_on_failure) { > die(_("failed to find exact merge base")); > } else { > + free_commit_list(merge_base); > free(rev); > return NULL; > } > } > > > @@ -2548,12 +2550,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > else > > print_signature(signature, rev.diffopt.file); > > } > > - if (output_directory) > > + if (output_directory) { > > fclose(rev.diffopt.file); > > + rev.diffopt.file = NULL; > > Is this a leakfix, or just a general code hygiene improvement? Not a leak fix, but required because of the leak fix. As we now unset `rev.diffopt.no_free`, `release_revisions()` will call `diff_free()` and try to close the file pointer. But as we already did, it would cause a segfault as we now try to close it twice. Patrick
diff --git a/builtin/log.c b/builtin/log.c index a73a767606..ff997a0d0e 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1833,6 +1833,7 @@ static struct commit *get_base_commit(const struct format_config *cfg, } rev[i] = merge_base->item; + free_commit_list(merge_base); } if (rev_nr % 2) @@ -2023,6 +2024,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) const char *rfc = NULL; int creation_factor = -1; const char *signature = git_version_string; + char *signature_to_free = NULL; char *signature_file_arg = NULL; struct keep_callback_data keep_callback_data = { .cfg = &cfg, @@ -2443,7 +2445,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (strbuf_read_file(&buf, signature_file, 128) < 0) die_errno(_("unable to read signature file '%s'"), signature_file); - signature = strbuf_detach(&buf, NULL); + signature = signature_to_free = strbuf_detach(&buf, NULL); } else if (cfg.signature) { signature = cfg.signature; } @@ -2548,12 +2550,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) else print_signature(signature, rev.diffopt.file); } - if (output_directory) + if (output_directory) { fclose(rev.diffopt.file); + rev.diffopt.file = NULL; + } } stop_progress(&progress); free(list); - free(branch_name); if (ignore_if_in_upstream) free_patch_ids(&ids); @@ -2565,11 +2568,14 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) strbuf_release(&rdiff_title); free(description_file); free(signature_file_arg); + free(signature_to_free); + free(branch_name); free(to_free); free(rev.message_id); if (rev.ref_message_ids) string_list_clear(rev.ref_message_ids, 0); free(rev.ref_message_ids); + rev.diffopt.no_free = 0; release_revisions(&rev); format_config_release(&cfg); return 0; diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 884f83fb8a..1c46e963e4 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -8,6 +8,7 @@ test_description='various format-patch tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh
There are various memory leaks hit by git-format-patch(1). Basically all of them are trivial, except that un-setting `diffopt.no_free` requires us to unset the `diffopt.file` because we manually close it already. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/log.c | 12 +++++++++--- t/t4014-format-patch.sh | 1 + 2 files changed, 10 insertions(+), 3 deletions(-)