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:29 UTC
- Message-ID
- <Yuqwa6NSG0Ee+7ev@coredump.intra.peff.net>
- In-Reply-To
- <YuqwCEqL4xZkxjL8@coredump.intra.peff.net>
On Wed, Aug 03, 2022 at 01:27:36PM -0400, Jeff King wrote:
Show 30 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.Ah, I see that you did add a comment in the next commit. That's sufficient, though I really think it makes more sense here, where we're actually dealing with leaks.
-Peff