Re: [PATCH v2 4/6] log: fix common "rev.pending" memory leak in "git show"

From: Jeff King <peff@peff.net>
Date: Jul 29, 2022, 18:55 UTC
Message-ID: <YuQtJ2DxNKX/y70N@coredump.intra.peff.net>
In-Reply-To: <patch-v2-4.6-54b632c1124-20220729T082919Z-avarab@gmail.com>

On Fri, Jul 29, 2022 at 10:31:06AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 8 quoted lines

> @@ -701,6 +701,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  		return cmd_log_deinit(cmd_log_walk(&rev), &rev);
>  
>  	memcpy(&pending, &rev.pending, sizeof(rev.pending));
> +	memcpy(&rev.pending, &blank, sizeof(rev.pending));
>  	rev.diffopt.no_free = 1;
>  	for (i = 0; i < pending.nr && !ret; i++) {
>  		struct object *o = pending.objects[i].item;

OK, so now "pending" is completely separate from what's in "rev". And then in the later hunk we clear it:

Show 5 quoted lines

> @@ -752,6 +752,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  			ret = error(_("unknown type: %d"), o->type);
>  		}
>  	}
> +	object_array_clear(&pending);

Good. But in the middle hunk...

Show 8 quoted lines

> @@ -744,7 +745,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  			rev.shown_one = 1;
>  			break;
>  		case OBJ_COMMIT:
> -			memcpy(&rev.pending, &blank, sizeof(rev.pending));
>  			add_object_array(o, name, &rev.pending);
>  			ret = cmd_log_walk_no_free(&rev);
>  			break;

We now do not do anything to clean up rev.pending. On the first pass, we'd see our blank pending array and add to it. But on a subsequent pass (i.e., because we are showing two or more commits), what will we see?

My initial assumption was we'd have the last pass's commit in "pending" here, so we'd be leaking it. But I think in practice it's OK. We end up in prepare_revision_walk(), which blanks the list again, and then processes each element. Non-commits _do_ end up back in the pending list, which would be a leak. But we know that this code triggers only for commits, which are placed only in the "commits" list (and that's cleaned up as we walk it via get_revision()).

That might be worth mentioning in the commit message, or even including a comment like:

  /*
   * No need to clean up rev.pending here; commits are removed from it
   * as part of prepare_revision_walk().
   */

-Peff