Re: [PATCH v3 3/6] log: fix a memory leak in "git show <revision>..."
From: Jeff King <hidden>
Date: 2022-08-03 17:27:40
On Tue, Aug 02, 2022 at 05:33:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
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.
quoted hunk ↗ jump to hunk
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