Re: [PATCH v3 3/6] log: fix a memory leak in "git show <revision>..."
- From
Jeff King <peff@peff.net>
- Date
- Aug 3, 2022, 17:27 UTC
- Message-ID
- <YuqwCEqL4xZkxjL8@coredump.intra.peff.net>
- In-Reply-To
- <patch-v3-3.6-83fc1835fe5-20220802T152925Z-avarab@gmail.com>
On Tue, Aug 02, 2022 at 05:33:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 8 quoted lines
> Let's fix this memory leak. Now when we encounter an OBJ_COMMIT we > save away the "rev.pending" before clearing it. We then add a single > commit to it, which our indirect invocation of prepare_revision_walk() > will remove. After that we restore the "rev.pending". > > Our "rev.pending" will then get free'd by the release_revisions() > added in f6bfea0ad01 (revisions API users: use release_revisions() in > builtin/log.c, 2022-04-13)
OK, I agree this is doing the correct thing. I actually found the earlier "let's dissociate rev.pending from rev entirely" approach easier to reason about, though.
Show 19 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c
> index 88a5e98875a..b4b1d974617 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)
> rev.shown_one = 1;
> break;
> case OBJ_COMMIT:
> + {
> + struct object_array old;
> +
> + memcpy(&old, &rev.pending, sizeof(old));
> rev.pending.nr = rev.pending.alloc = 0;
> rev.pending.objects = NULL;
> add_object_array(o, name, &rev.pending);
> ret = cmd_log_walk_no_free(&rev);
> + memcpy(&rev.pending, &old, sizeof(rev.pending));
> break;
> + }Here we overwrite the one-item rev.pending without freeing it, but just immediately after instead of before. It's a little subtle, but your comment in the commit message:
[...] and only free the new "rev.pending" in the "OBJ_COMMIT" case arm as prepare_revision_walk() would draw it down.
covers that. IMHO that could be spelled out a bit more (particularly that this only works for OBJ_COMMIT, but that's OK because that's the only type we're adding), but I can live with it.
-Peff