Re: [PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

From: Jeff King <peff@peff.net>
Date: Jun 24, 2021, 16:54 UTC
Message-ID: <YNS4wCjkp4yiLLBQ@coredump.intra.peff.net>
In-Reply-To: <patch-1.3-932c0883ce0-20210621T151357Z-avarab@gmail.com>

On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 10 quoted lines

> Fix a memory leak from the prefix_filename() function introduced with
> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
> 2017-03-20).
> 
> As noted in that commit the leak was intentional as a part of being
> sloppy about freeing resources just before we exit, I'm changing this
> because I'll be fixing other memory leaks in the bundle API (including
> the library version) in subsequent commits. It's easier to reason
> about those fixes if valgrind runs cleanly at the end without any
> leaks whatsoever.

Looking at that old commit, it seems like this is a good candidate for just inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it looks like the allocation has now migrated into all of the individual sub-command functions, so we have to deal with it multiple times. They could still use UNLEAK() if you want to avoid the "ret = foo(); free(); return ret" dance in each one, though.

We should avoid UNLEAK() in library-ish functions, but sub-commands that are just one step away from cmd_bundle() returning are OK uses, IMHO.

Show 17 quoted lines

> @@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  	if (progress && all_progress_implied)
>  		strvec_push(&pack_opts, "--all-progress-implied");
>  
> -	if (!startup_info->have_repository)
> +	if (!startup_info->have_repository) {
> +		die_no_repo = 1;
> +		goto cleanup;
> +	}
> +	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
> +cleanup:
> +	free(bundle_file);
> +	if (die_no_repo)
>  		die(_("Need a repository to create a bundle."));
> -	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
> +	return ret;
>  }

This die_no_repo stuff confused me at first. But I think you are trying to make sure we call free(bundle_file) before die? There is no point in spending any effort on that, I think. When we exit() via die(), the variable is still on the stack, and hence not leaked. And there are probably a zillion other places we can hit a die() inside create_bundle() anyway, which would produce the same effect. There's not much point treating this one specially.

-Peff