git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, James Liu <james@jamesliu.io>,
	karthik nayak <karthik.188@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v3 18/22] builtin/format-patch: fix various trivial memory leaks
Date: Wed, 14 Aug 2024 06:56:25 +0200	[thread overview]
Message-ID: <Zrw47_cAtpVZ0Inh@tanuki> (raw)
In-Reply-To: <xmqqed6s747l.fsf@gitster.g>

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

  reply	other threads:[~2024-08-14  4:56 UTC|newest]

Thread overview: 146+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-06  8:59 [PATCH 00/22] Memory leak fixes (pt.4) Patrick Steinhardt
2024-08-06  8:59 ` [PATCH 01/22] remote: plug memory leak when aliasing URLs Patrick Steinhardt
2024-08-06  8:59 ` [PATCH 02/22] git: fix leaking system paths Patrick Steinhardt
2024-08-07  4:02   ` James Liu
2024-08-06  8:59 ` [PATCH 03/22] object-file: fix memory leak when reading corrupted headers Patrick Steinhardt
2024-08-06  8:59 ` [PATCH 04/22] object-name: fix leaking symlink paths in object context Patrick Steinhardt
2024-08-06  8:59 ` [PATCH 05/22] bulk-checkin: fix leaking state TODO Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 06/22] read-cache: fix leaking hashfile when writing index fails Patrick Steinhardt
2024-08-07  7:01   ` James Liu
2024-08-08  5:04     ` Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 07/22] submodule-config: fix leaking name enrty when traversing submodules Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 08/22] config: fix leaking comment character config Patrick Steinhardt
2024-08-07  7:11   ` James Liu
2024-08-08  5:04     ` Patrick Steinhardt
2024-08-08 15:54       ` Junio C Hamano
2024-08-06  9:00 ` [PATCH 09/22] builtin/rebase: fix leaking `commit.gpgsign` value Patrick Steinhardt
2024-08-07  7:32   ` James Liu
2024-08-08  5:05     ` Patrick Steinhardt
2024-08-08 10:07   ` Phillip Wood
2024-08-08 12:58     ` Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 10/22] builtin/notes: fix leaking `struct notes_tree` when merging notes Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 11/22] builtin/fast-import: plug trivial memory leaks Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 12/22] builtin/fast-export: fix leaking diff options Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 13/22] builtin/fast-export: plug leaking tag names Patrick Steinhardt
2024-08-07  8:31   ` James Liu
2024-08-08  5:05     ` Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 14/22] merge-ort: unconditionally release attributes index Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 15/22] sequencer: release todo list on error paths Patrick Steinhardt
2024-08-08 10:08   ` Phillip Wood
2024-08-08 16:31     ` Junio C Hamano
2024-08-06  9:00 ` [PATCH 16/22] unpack-trees: clear index when not propagating it Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 17/22] diff: fix leak when parsing invalid ignore regex option Patrick Steinhardt
2024-08-06  9:00 ` [PATCH 18/22] builtin/format-patch: fix various trivial memory leaks Patrick Steinhardt
2024-08-07  8:51   ` James Liu
2024-08-08  5:05     ` Patrick Steinhardt
2024-08-06  9:01 ` [PATCH 19/22] userdiff: fix leaking memory for configured diff drivers Patrick Steinhardt
2024-08-07  9:25   ` James Liu
2024-08-08  5:05     ` Patrick Steinhardt
2024-08-08 16:05       ` Junio C Hamano
2024-08-06  9:01 ` [PATCH 20/22] builtin/log: fix leak when showing converted blob contents Patrick Steinhardt
2024-08-06  9:01 ` [PATCH 21/22] diff: free state populated via options Patrick Steinhardt
2024-08-06  9:01 ` [PATCH 22/22] builtin/diff: free symmetric diff members Patrick Steinhardt
2024-08-07  9:27 ` [PATCH 00/22] Memory leak fixes (pt.4) James Liu
2024-08-08  5:05   ` Patrick Steinhardt
2024-08-08  6:00     ` James Liu
2024-08-07 16:59 ` Junio C Hamano
2024-08-07 17:03   ` Patrick Steinhardt
2024-08-08  0:32     ` Junio C Hamano
2024-08-08 13:04 ` [PATCH v2 " Patrick Steinhardt
2024-08-08 13:04   ` [PATCH v2 01/22] remote: plug memory leak when aliasing URLs Patrick Steinhardt
2024-08-12  8:27     ` karthik nayak
2024-08-12 14:08     ` Taylor Blau
2024-08-12 14:37     ` Jeff King
2024-08-13  6:34       ` Patrick Steinhardt
2024-08-08 13:04   ` [PATCH v2 02/22] git: fix leaking system paths Patrick Steinhardt
2024-08-12 14:11     ` Taylor Blau
2024-08-13  6:30       ` Patrick Steinhardt
2024-08-13 16:02         ` Junio C Hamano
2024-08-08 13:04   ` [PATCH v2 03/22] object-file: fix memory leak when reading corrupted headers Patrick Steinhardt
2024-08-12  8:43     ` karthik nayak
2024-08-08 13:04   ` [PATCH v2 04/22] object-name: fix leaking symlink paths in object context Patrick Steinhardt
2024-08-08 13:04   ` [PATCH v2 05/22] bulk-checkin: fix leaking state TODO Patrick Steinhardt
2024-08-08 13:04   ` [PATCH v2 06/22] read-cache: fix leaking hashfile when writing index fails Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 07/22] submodule-config: fix leaking name enrty when traversing submodules Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 08/22] config: fix leaking comment character config Patrick Steinhardt
2024-08-08 17:12     ` Junio C Hamano
2024-08-12  7:45       ` Patrick Steinhardt
2024-08-12 20:32         ` Junio C Hamano
2024-08-13  6:54           ` Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 09/22] builtin/rebase: fix leaking `commit.gpgsign` value Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 10/22] builtin/notes: fix leaking `struct notes_tree` when merging notes Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 11/22] builtin/fast-import: plug trivial memory leaks Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 12/22] builtin/fast-export: fix leaking diff options Patrick Steinhardt
2024-08-12  9:05     ` karthik nayak
2024-08-08 13:05   ` [PATCH v2 13/22] builtin/fast-export: plug leaking tag names Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 14/22] merge-ort: unconditionally release attributes index Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 15/22] sequencer: release todo list on error paths Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 16/22] unpack-trees: clear index when not propagating it Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 17/22] diff: fix leak when parsing invalid ignore regex option Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 18/22] builtin/format-patch: fix various trivial memory leaks Patrick Steinhardt
2024-08-08 13:05   ` [PATCH v2 19/22] userdiff: fix leaking memory for configured diff drivers Patrick Steinhardt
2024-08-08 13:06   ` [PATCH v2 20/22] builtin/log: fix leak when showing converted blob contents Patrick Steinhardt
2024-08-08 13:06   ` [PATCH v2 21/22] diff: free state populated via options Patrick Steinhardt
2024-08-08 13:06   ` [PATCH v2 22/22] builtin/diff: free symmetric diff members Patrick Steinhardt
2024-08-12  9:12     ` karthik nayak
2024-08-12  9:13   ` [PATCH v2 00/22] Memory leak fixes (pt.4) karthik nayak
2024-08-12 15:49     ` Junio C Hamano
2024-08-13  6:27       ` Patrick Steinhardt
2024-08-12 14:01   ` Phillip Wood
2024-08-12 15:50     ` Junio C Hamano
2024-08-13  9:31 ` [PATCH v3 " Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 01/22] remote: plug memory leak when aliasing URLs Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 02/22] git: fix leaking system paths Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 03/22] object-file: fix memory leak when reading corrupted headers Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 04/22] object-name: fix leaking symlink paths in object context Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 05/22] bulk-checkin: fix leaking state TODO Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 06/22] read-cache: fix leaking hashfile when writing index fails Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 07/22] submodule-config: fix leaking name entry when traversing submodules Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 08/22] config: fix leaking comment character config Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 09/22] builtin/rebase: fix leaking `commit.gpgsign` value Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 10/22] builtin/notes: fix leaking `struct notes_tree` when merging notes Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 11/22] builtin/fast-import: plug trivial memory leaks Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 12/22] builtin/fast-export: fix leaking diff options Patrick Steinhardt
2024-08-13 16:34     ` Junio C Hamano
2024-08-14  4:49       ` Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 13/22] builtin/fast-export: plug leaking tag names Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 14/22] merge-ort: unconditionally release attributes index Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 15/22] sequencer: release todo list on error paths Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 16/22] unpack-trees: clear index when not propagating it Patrick Steinhardt
2024-08-13  9:31   ` [PATCH v3 17/22] diff: fix leak when parsing invalid ignore regex option Patrick Steinhardt
2024-08-13  9:32   ` [PATCH v3 18/22] builtin/format-patch: fix various trivial memory leaks Patrick Steinhardt
2024-08-13 16:55     ` Junio C Hamano
2024-08-14  4:56       ` Patrick Steinhardt [this message]
2024-08-13 16:55     ` Junio C Hamano
2024-08-13  9:32   ` [PATCH v3 19/22] userdiff: fix leaking memory for configured diff drivers Patrick Steinhardt
2024-08-13  9:32   ` [PATCH v3 20/22] builtin/log: fix leak when showing converted blob contents Patrick Steinhardt
2024-08-13  9:32   ` [PATCH v3 21/22] diff: free state populated via options Patrick Steinhardt
2024-08-13 16:31     ` Junio C Hamano
2024-08-13  9:32   ` [PATCH v3 22/22] builtin/diff: free symmetric diff members Patrick Steinhardt
2024-08-13 16:25     ` Junio C Hamano
2024-08-14  5:01       ` Patrick Steinhardt
2024-08-14 15:28         ` Junio C Hamano
2024-08-13 16:58   ` [PATCH v3 00/22] Memory leak fixes (pt.4) Junio C Hamano
2024-08-14  6:51 ` [PATCH v4 " Patrick Steinhardt
2024-08-14  6:51   ` [PATCH v4 01/22] remote: plug memory leak when aliasing URLs Patrick Steinhardt
2024-08-14  6:51   ` [PATCH v4 02/22] git: fix leaking system paths Patrick Steinhardt
2024-08-14  6:51   ` [PATCH v4 03/22] object-file: fix memory leak when reading corrupted headers Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 04/22] object-name: fix leaking symlink paths in object context Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 05/22] bulk-checkin: fix leaking state TODO Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 06/22] read-cache: fix leaking hashfile when writing index fails Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 07/22] submodule-config: fix leaking name entry when traversing submodules Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 08/22] config: fix leaking comment character config Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 09/22] builtin/rebase: fix leaking `commit.gpgsign` value Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 10/22] builtin/notes: fix leaking `struct notes_tree` when merging notes Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 11/22] builtin/fast-import: plug trivial memory leaks Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 12/22] builtin/fast-export: fix leaking diff options Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 13/22] builtin/fast-export: plug leaking tag names Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 14/22] merge-ort: unconditionally release attributes index Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 15/22] sequencer: release todo list on error paths Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 16/22] unpack-trees: clear index when not propagating it Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 17/22] diff: fix leak when parsing invalid ignore regex option Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 18/22] builtin/format-patch: fix various trivial memory leaks Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 19/22] userdiff: fix leaking memory for configured diff drivers Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 20/22] builtin/log: fix leak when showing converted blob contents Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 21/22] diff: free state populated via options Patrick Steinhardt
2024-08-14  6:52   ` [PATCH v4 22/22] builtin/diff: free symmetric diff members Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Zrw47_cAtpVZ0Inh@tanuki \
    --to=ps@pks.im \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=james@jamesliu.io \
    --cc=karthik.188@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=phillip.wood123@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).