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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jun 24 2021, Jeff King wrote:

> On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.

I'll change it if you feel strongly about it, but I always read UNLEAK()
as "ok, this is too hard, we won't bother, it's just a one-off
built-in", and not necessarily a recommendation for a desired pattern.

I think it's nice to have e.g. valgrind be able to report no leaks in
the binaries we build by default, not just if you compile with
-DSUPPRESS_ANNOTATED_LEAKS.

>> @@ -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.

Right, it's there just for the free(), and yeah, there's a bunch of
places we'll leak anyway.

I do think per the above with valgrind that there's value in making the
common non-dying codepaths not leak though.