threads / patch / 48999

patchremote: clear string_list after use in mv()

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

## tl;dr

3 messages between Aug 1, 2018 and Aug 2, 2018. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

René Scharfe· Aug 1, 2018, 10:19 UTC · lore

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(-)
Show changes to builtin/remote.c +3 −2
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
Jonathan Nieder· Aug 2, 2018, 02:56 UTC · re: René Scharfe · lore

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

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.
René Scharfe· Aug 2, 2018, 09:58 UTC · re: Jonathan Nieder · lore

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

Thank you for the review!
Am 02.08.2018 um 04:56 schrieb Jonathan Nieder:
Show 10 quoted lines
> 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.

Show 7 quoted lines
>> --- 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.

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

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.)
Show 10 quoted lines
>> +	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é

← back to recent threads