Re: [PATCH] remote: clear string_list after use in mv()

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 2, 2018, 02:56 UTC
Message-ID: <20180802025641.GF189024@aiede.svl.corp.google.com>
In-Reply-To: <553f84f2-7d9d-9a92-d5ca-39bd862ff748@web.de>

Hi,

René Scharfe wrote:

> Subject: remote: clear string_list after use in mv()

This subject line doesn't fully reflect the goodness of this change. How about something like:

	remote mv: avoid leaking ref names in string_list

Show 13 quoted lines

> Switch to the _DUP variant of string_list for remote_branches to allow
> string_list_clear() to release the allocated memory at the end, and
> actually call that function.  Free the util pointer as well; it is
> allocated in read_remote_branches().
>
> NB: This string_list is empty until read_remote_branches() is called
> via for_each_ref(), so there is no need to clean it up when returning
> before that point.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Patch generated with ---function-context for easier review -- that
> makes it look much bigger than it actually is, though.

Thanks, that indeed helps.

[...]

> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -558,23 +558,23 @@ struct rename_info {

optional: Would it be worth a comment in the struct definition to say this string_list owns its items (or in other words that it's a _DUP string_list)?

I think we're fine without, since it's local to the file, but may make sense to do if rerolling.

[...]

Show 11 quoted lines

> @@ -607,133 +607,134 @@ static int migrate_file(struct remote *remote)
>  static int mv(int argc, const char **argv)
>  {
>  	struct option options[] = {
>  		OPT_END()
>  	};
>  	struct remote *oldremote, *newremote;
>  	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,
>  		old_remote_context = STRBUF_INIT;
> -	struct string_list remote_branches = STRING_LIST_INIT_NODUP;
> +	struct string_list remote_branches = STRING_LIST_INIT_DUP;

Verified that these are the only two functions that touch the remote_branches field. This patch didn't miss any callers.

[...]

Show 8 quoted lines

>  	if (!refspec_updated)
>  		return 0;
>  
>  	/*
>  	 * First remove symrefs, then rename the rest, finally create
>  	 * the new symrefs.
>  	 */
>  	for_each_ref(read_remote_branches, &rename);

As you noted, this is the first caller that writes to the string_list, so we don't have to worry about the 'return 0' above. That said, I wonder if it might be simpler and more futureproof to use destructor-style cleanup handling anyway:

	if (!refspec_updated)
		goto done;
 [...]
  done:
	string_list_clear(&remote_branches, 1);
	return 0;

[...]

> +	string_list_clear(&remote_branches, 1);

not about this patch: I wonder if we should make the free_util parameter a flag word so the behavior is more obvious in the caller:

	string_list_clear(&remote_branches, STRING_LIST_FREE_UTIL);

Or maybe even having a separate function:

	string_list_clear_free_util(&remote_branches);

That's a topic for another day, though.

With whatever subset of the changes suggested above makes sense,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for a pleasant read.