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

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

[René Scharfe]

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.

 builtin/remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index c74ee88690..07bd51f8eb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -558,23 +558,23 @@ struct rename_info {
 static int read_remote_branches(const char *refname,
 	const struct object_id *oid, int flags, void *cb_data)
 {
 	struct rename_info *rename = cb_data;
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list_item *item;
 	int flag;
 	const char *symref;
 
 	strbuf_addf(&buf, "refs/remotes/%s/", rename->old_name);
 	if (starts_with(refname, buf.buf)) {
-		item = string_list_append(rename->remote_branches, xstrdup(refname));
+		item = string_list_append(rename->remote_branches, refname);
 		symref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,
 					    NULL, &flag);
 		if (symref && (flag & REF_ISSYMREF))
 			item->util = xstrdup(symref);
 		else
 			item->util = NULL;
 	}
 	strbuf_release(&buf);
 
 	return 0;
 }
@@ -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;
 	struct rename_info rename;
 	int i, refspec_updated = 0;
 
 	if (argc != 3)
 		usage_with_options(builtin_remote_rename_usage, options);
 
 	rename.old_name = argv[1];
 	rename.new_name = argv[2];
 	rename.remote_branches = &remote_branches;
 
 	oldremote = remote_get(rename.old_name);
 	if (!remote_is_configured(oldremote, 1))
 		die(_("No such remote: %s"), rename.old_name);
 
 	if (!strcmp(rename.old_name, rename.new_name) && oldremote->origin != REMOTE_CONFIG)
 		return migrate_file(oldremote);
 
 	newremote = remote_get(rename.new_name);
 	if (remote_is_configured(newremote, 1))
 		die(_("remote %s already exists."), rename.new_name);
 
 	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new_name);
 	if (!valid_fetch_refspec(buf.buf))
 		die(_("'%s' is not a valid remote name"), rename.new_name);
 
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s", rename.old_name);
 	strbuf_addf(&buf2, "remote.%s", rename.new_name);
 	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
 		return error(_("Could not rename config section '%s' to '%s'"),
 				buf.buf, buf2.buf);
 
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.fetch", rename.new_name);
 	git_config_set_multivar(buf.buf, NULL, NULL, 1);
 	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name);
 	for (i = 0; i < oldremote->fetch.raw_nr; i++) {
 		char *ptr;
 
 		strbuf_reset(&buf2);
 		strbuf_addstr(&buf2, oldremote->fetch.raw[i]);
 		ptr = strstr(buf2.buf, old_remote_context.buf);
 		if (ptr) {
 			refspec_updated = 1;
 			strbuf_splice(&buf2,
 				      ptr-buf2.buf + strlen(":refs/remotes/"),
 				      strlen(rename.old_name), rename.new_name,
 				      strlen(rename.new_name));
 		} else
 			warning(_("Not updating non-default fetch refspec\n"
 				  "\t%s\n"
 				  "\tPlease update the configuration manually if necessary."),
 				buf2.buf);
 
 		git_config_set_multivar(buf.buf, buf2.buf, "^$", 0);
 	}
 
 	read_branches();
 	for (i = 0; i < branch_list.nr; i++) {
 		struct string_list_item *item = branch_list.items + i;
 		struct branch_info *info = item->util;
 		if (info->remote_name && !strcmp(info->remote_name, rename.old_name)) {
 			strbuf_reset(&buf);
 			strbuf_addf(&buf, "branch.%s.remote", item->string);
 			git_config_set(buf.buf, rename.new_name);
 		}
 	}
 
 	if (!refspec_updated)
 		return 0;
 
 	/*
 	 * First remove symrefs, then rename the rest, finally create
 	 * the new symrefs.
 	 */
 	for_each_ref(read_remote_branches, &rename);
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
 		int flag = 0;
 		struct object_id oid;
 
 		read_ref_full(item->string, RESOLVE_REF_READING, &oid, &flag);
 		if (!(flag & REF_ISSYMREF))
 			continue;
 		if (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))
 			die(_("deleting '%s' failed"), item->string);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
 
 		if (item->util)
 			continue;
 		strbuf_reset(&buf);
 		strbuf_addstr(&buf, item->string);
 		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old_name),
 				rename.new_name, strlen(rename.new_name));
 		strbuf_reset(&buf2);
 		strbuf_addf(&buf2, "remote: renamed %s to %s",
 				item->string, buf.buf);
 		if (rename_ref(item->string, buf.buf, buf2.buf))
 			die(_("renaming '%s' failed"), item->string);
 	}
 	for (i = 0; i < remote_branches.nr; i++) {
 		struct string_list_item *item = remote_branches.items + i;
 
 		if (!item->util)
 			continue;
 		strbuf_reset(&buf);
 		strbuf_addstr(&buf, item->string);
 		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old_name),
 				rename.new_name, strlen(rename.new_name));
 		strbuf_reset(&buf2);
 		strbuf_addstr(&buf2, item->util);
 		strbuf_splice(&buf2, strlen("refs/remotes/"), strlen(rename.old_name),
 				rename.new_name, strlen(rename.new_name));
 		strbuf_reset(&buf3);
 		strbuf_addf(&buf3, "remote: renamed %s to %s",
 				item->string, buf.buf);
 		if (create_symref(buf.buf, buf2.buf, buf3.buf))
 			die(_("creating '%s' failed"), buf.buf);
 	}
+	string_list_clear(&remote_branches, 1);
 	return 0;
 }
 
-- 
2.18.0

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

[Jonathan Nieder]

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

?

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

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

[...]
>  	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.

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

[René Scharfe]

Thank you for the review!

Am 02.08.2018 um 04:56 schrieb Jonathan Nieder:
> 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
> 
> ?

Hmm, "clearing" a string_list is how we release its memory, and since we
didn't do it before (otherwise we wouldn't need the patch) it leaked
before.  So I think the title implies plugging a leak already.

The ref names don't take up much space -- they are probably in the range
of hundreds to thousands and probably take up less than 100 bytes each.
The command exits after calling mv(), so this is a minor leak which
won't be noticed by users.

The benefit of this patch is to bring this function in line with its
siblings, which all release their string_lists when done.

>> --- 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)?

Such a comment could easily get out of sync with the assignment later in
the code.  And the struct could easily be used with both types of
string_lists, even if that's not the case here, so I don't think that's
the right place.

We could add an assert(rename->remote_branches->strdup_strings) before
the string_list_append() call instead, which would document that
requirement in the right place in a way that shouldn't get out of sync
silently, but I'm not sure it's worth it here.

>>   	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;

There are some more early returns higher up, which would have to be
adjusted as well.  Such a restructuring would be helpful if we decide
to release the various strbufs in that function as well..

But perhaps the main problem is that the function is too long. Could
it be split up into sensible parts with a lower number of allocations
each that can be kept track of more easily?

(Sounds like a bigger bite than I can chew at the moment, though.)

>> +	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);

I agree that naming variants instead of using binary options is a good
idea in general, as it makes the code self-documenting.

I'd like to suggest another option: remove the second parameter of
string_list_clear() and add string_list_free_util(), which only free(3)s
->util pointers.  Users that attached heap objects to string_list items
would have to call both functions; no need to glue them together.

René