Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> When calling `git describe --contains=`, we end up invoking
> `cmd_name_rev()` with some munged argv array. This array may contain
> allocated strings and furthermore will likely be modified by the called
> function. This results in two memory leaks:
>
>   - First, we leak the array that we use to assemble the arguments.
>
>   - Second, we leak the allocated strings that we may have put into the
>     array.
>
> Fix those leaks by creating a separate copy of the array that we can
> hand over to `cmd_name_rev()`. This allows us to free all strings
> contained in the `strvec`, as the original vector will not be modified
> anymore.

OK, the separate copy has to be a shallow copy, as its purpose is
not to lose pointers to the contained strings.

> Furthermore, free both the `strvec` and the copied array to fix the
> first memory leak.
> ...
> +		strvec_clear(&args);
> +		free(argv_copy);

So, calling cmd_name_rev() may shuffle the argv_copy[] array but at
least it will not free any element in it (as expected---it is
typically the (argc, argv[]) the process receives from getting
exec'ed) [*].  Freeing the argv_copy shell itself is sufficient to
discard what we used to call cmd_name_rev().  And we discard args
both its content strings and the array.  OK.

> +		return ret;
>  	}
>  
>  	hashmap_init(&names, commit_name_neq, NULL, 0);


[Footnote]

 * The fact that cmd_foo() is called is not a hygiene thing to do in
   the first place, and in the longer term #leftoverbits we may need
   to refactor the thing further, into a proper library-ish reusable
   helper function that can be used to compute name_rev() any number
   of times, plus cmd_name_rev() and this caller that call it.