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