[PATCH] remote: plug memory leaks at early returns

[PATCH] remote: plug memory leaks at early returns

[René Scharfe]

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

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

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

[Junio C Hamano]

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.

> 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

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

[René Scharfe]

Am 23.08.24 um 23:13 schrieb Junio C Hamano:
> 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é

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

[Patrick Steinhardt]

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!

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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