threads / patch / 61999

patchremote: plug memory leaks at early returns

Subject: [PATCH] remote: plug memory leaks at early returns

## tl;dr

5 messages between Aug 23, 2024 and Aug 26, 2024. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

René Scharfe· Aug 23, 2024, 20:21 UTC · lore
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/remote.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)
Show changes to builtin/remote.c +15 −9
diff --git a/builtin/remote.c b/builtin/remote.c
index d1f9292ed2..0acc547d69 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -164,6 +164,7 @@ static int add(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
 	const char *name, *url;
 	int i;
+	int result = 0;

 	struct option options[] = {
 		OPT_BOOL('f', "fetch", &fetch, N_("fetch the remote branches")),
@@ -230,8 +231,10 @@ static int add(int argc, const char **argv, const char *prefix)
 			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
 	}

-	if (fetch && fetch_remote(name))
-		return 1;
+	if (fetch && fetch_remote(name)) {
+		result = 1;
+		goto out;
+	}

 	if (master) {
 		strbuf_reset(&buf);
@@ -241,14 +244,15 @@ static int add(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master);

 		if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote add"))
-			return error(_("Could not setup master '%s'"), master);
+			result = error(_("Could not setup master '%s'"), master);
 	}

+out:
 	strbuf_release(&buf);
 	strbuf_release(&buf2);
 	string_list_clear(&track, 0);

-	return 0;
+	return result;
 }

 struct branch_info {
@@ -715,6 +719,7 @@ static int mv(int argc, const char **argv, const char *prefix)
 	struct rename_info rename;
 	int i, refs_renamed_nr = 0, refspec_updated = 0;
 	struct progress *progress = NULL;
+	int result = 0;

 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_rename_usage, 0);
@@ -747,9 +752,11 @@ static int mv(int argc, const char **argv, const char *prefix)

 	strbuf_addf(&buf, "remote.%s", rename.old_name);
 	strbuf_addf(&buf2, "remote.%s", rename.new_name);
-	if (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1)
-		return error(_("Could not rename config section '%s' to '%s'"),
-				buf.buf, buf2.buf);
+	if (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1) {
+		result = error(_("Could not rename config section '%s' to '%s'"),
+			       buf.buf, buf2.buf);
+		goto out;
+	}

 	if (oldremote->fetch.raw_nr) {
 		strbuf_reset(&buf);
@@ -870,7 +877,7 @@ static int mv(int argc, const char **argv, const char *prefix)
 	strbuf_release(&buf);
 	strbuf_release(&buf2);
 	strbuf_release(&buf3);
-	return 0;
+	return result;
 }

 static int rm(int argc, const char **argv, const char *prefix)
--
2.30.2
Junio C Hamano· Aug 23, 2024, 21:13 UTC · re: René Scharfe · lore

Re: [PATCH] remote: plug memory leaks at early returns

René Scharfe <l.s.r@web.de> writes:
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/remote.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

Looks straight-forward. Does this allow us to mark any test script as leak-free? I understand that Patrick has another round of leakfixes topic that is not yet published, and I'd prefer to see us not step each other's toes.

Will queue in the meantime but may drop it if Patrick says it already is covered or something.

Thanks.
Show 77 quoted lines
> diff --git a/builtin/remote.c b/builtin/remote.c
> index d1f9292ed2..0acc547d69 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -164,6 +164,7 @@ static int add(int argc, const char **argv, const char *prefix)
>  	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
>  	const char *name, *url;
>  	int i;
> +	int result = 0;
>
>  	struct option options[] = {
>  		OPT_BOOL('f', "fetch", &fetch, N_("fetch the remote branches")),
> @@ -230,8 +231,10 @@ static int add(int argc, const char **argv, const char *prefix)
>  			       fetch_tags == TAGS_SET ? "--tags" : "--no-tags");
>  	}
>
> -	if (fetch && fetch_remote(name))
> -		return 1;
> +	if (fetch && fetch_remote(name)) {
> +		result = 1;
> +		goto out;
> +	}
>
>  	if (master) {
>  		strbuf_reset(&buf);
> @@ -241,14 +244,15 @@ static int add(int argc, const char **argv, const char *prefix)
>  		strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master);
>
>  		if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote add"))
> -			return error(_("Could not setup master '%s'"), master);
> +			result = error(_("Could not setup master '%s'"), master);
>  	}
>
> +out:
>  	strbuf_release(&buf);
>  	strbuf_release(&buf2);
>  	string_list_clear(&track, 0);
>
> -	return 0;
> +	return result;
>  }
>
>  struct branch_info {
> @@ -715,6 +719,7 @@ static int mv(int argc, const char **argv, const char *prefix)
>  	struct rename_info rename;
>  	int i, refs_renamed_nr = 0, refspec_updated = 0;
>  	struct progress *progress = NULL;
> +	int result = 0;
>
>  	argc = parse_options(argc, argv, prefix, options,
>  			     builtin_remote_rename_usage, 0);
> @@ -747,9 +752,11 @@ static int mv(int argc, const char **argv, const char *prefix)
>
>  	strbuf_addf(&buf, "remote.%s", rename.old_name);
>  	strbuf_addf(&buf2, "remote.%s", rename.new_name);
> -	if (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1)
> -		return error(_("Could not rename config section '%s' to '%s'"),
> -				buf.buf, buf2.buf);
> +	if (repo_config_rename_section(the_repository, buf.buf, buf2.buf) < 1) {
> +		result = error(_("Could not rename config section '%s' to '%s'"),
> +			       buf.buf, buf2.buf);
> +		goto out;
> +	}
>
>  	if (oldremote->fetch.raw_nr) {
>  		strbuf_reset(&buf);
> @@ -870,7 +877,7 @@ static int mv(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&buf);
>  	strbuf_release(&buf2);
>  	strbuf_release(&buf3);
> -	return 0;
> +	return result;
>  }
>
>  static int rm(int argc, const char **argv, const char *prefix)
> --
> 2.30.2
René Scharfe· Aug 24, 2024, 08:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] remote: plug memory leaks at early returns

Am 23.08.24 um 23:13 schrieb Junio C Hamano:
Show 8 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  builtin/remote.c | 23 +++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> Does this allow us to mark any test script as leak-free?

I don't think so -- at least GIT_TEST_PASSING_SANITIZE_LEAK=check still passes for me.

> Will queue in the meantime but may drop it if Patrick says it
> already is covered or something.
OK.
René
Patrick Steinhardt· Aug 26, 2024, 05:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] remote: plug memory leaks at early returns

On Fri, Aug 23, 2024 at 02:13:09PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> René Scharfe <l.s.r@web.de> writes:
> 
> > Signed-off-by: René Scharfe <l.s.r@web.de>
> > ---
> >  builtin/remote.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> Looks straight-forward.  Does this allow us to mark any test script
> as leak-free?  I understand that Patrick has another round of
> leakfixes topic that is not yet published, and I'd prefer to see us
> not step each other's toes.

No, this doesn't conflict with anything I have. And even if it did, I'd be happy to drop some patches from my local series :)

The changes themselves also look good to me, thanks!
Patrick
Junio C Hamano· Aug 26, 2024, 15:11 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] remote: plug memory leaks at early returns

Patrick Steinhardt <ps@pks.im> writes:
Show 17 quoted lines
> On Fri, Aug 23, 2024 at 02:13:09PM -0700, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>> 
>> > Signed-off-by: René Scharfe <l.s.r@web.de>
>> > ---
>> >  builtin/remote.c | 23 +++++++++++++++--------
>> >  1 file changed, 15 insertions(+), 8 deletions(-)
>> 
>> Looks straight-forward.  Does this allow us to mark any test script
>> as leak-free?  I understand that Patrick has another round of
>> leakfixes topic that is not yet published, and I'd prefer to see us
>> not step each other's toes.
>
> No, this doesn't conflict with anything I have. And even if it did, I'd
> be happy to drop some patches from my local series :)
>
> The changes themselves also look good to me, thanks!
Thanks.  Will mark for "next".

← back to recent threads