threads / patch / 46874

patchbuiltin/: add UNLEAKs

Subject: [PATCH] builtin/: add UNLEAKs

## tl;dr

5 messages between Oct 1, 2017 and Oct 3, 2017. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Martin Ågren· Oct 1, 2017, 17:42 UTC · lore

Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the variables in the same order as we've declared them. While addressing `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as well.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/checkout.c   | 1 +
 builtin/diff-index.c | 1 +
 builtin/diff.c       | 3 +++
 builtin/name-rev.c   | 1 +
 builtin/tag.c        | 9 +++++----
 5 files changed, 11 insertions(+), 4 deletions(-)

Show changes to 5 files +11 −4

builtin/checkout.c, builtin/diff-index.c, builtin/diff.c, builtin/name-rev.c, builtin/tag.c

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3345a0d16..2daa4412a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1298,6 +1298,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
+	UNLEAK(opts);
 	if (opts.patch_mode || opts.pathspec.nr)
 		return checkout_paths(&opts, new.name);
 	else
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 9d772f8f2..522f4fdff 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -56,5 +56,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		return -1;
 	}
 	result = run_diff_index(&rev, cached);
+	UNLEAK(rev);
 	return diff_result_code(&rev.diffopt, result);
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index 7e3ebcea3..f5bbd4d75 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -464,5 +464,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	result = diff_result_code(&rev.diffopt, result);
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
+	UNLEAK(rev);
+	UNLEAK(ent);
+	UNLEAK(blob);
 	return result;
 }
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 598da6c8b..9e088ebd1 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -494,5 +494,6 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 				  always, allow_undefined, data.name_only);
 	}
 
+	UNLEAK(revs);
 	return 0;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index c62779418..34efba579 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -552,9 +552,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (force && !is_null_oid(&prev) && oidcmp(&prev, &object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev.hash, DEFAULT_ABBREV));
 
-	strbuf_release(&err);
-	strbuf_release(&buf);
-	strbuf_release(&ref);
-	strbuf_release(&reflog_msg);
+	UNLEAK(buf);
+	UNLEAK(ref);
+	UNLEAK(reflog_msg);
+	UNLEAK(msg);
+	UNLEAK(err);
 	return 0;
 }

-- 
2.14.2.666.gea220ee40

Junio C Hamano· Oct 2, 2017, 04:07 UTC · re: Martin Ågren · lore

Re: [PATCH] builtin/: add UNLEAKs

Martin Ågren <martin.agren@gmail.com> writes:

> ... While addressing `msg` in builtin/tag.c, convert the existing
> `strbuf_release()` calls as well.

That part of this patch made me raise eyebrows a bit but only slightly. We are about to leave the function to exit anyway, and all the other that got new UNLEAK(var) at the end are doing the same thing.

So overall I think this patch makes sense.  Thanks.

Jeff King· Oct 2, 2017, 06:25 UTC · re: Martin Ågren · lore

Re: [PATCH] builtin/: add UNLEAKs

On Sun, Oct 01, 2017 at 07:42:08PM +0200, Martin Ågren wrote:

> Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the
> variables in the same order as we've declared them. While addressing
> `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as
> well.

It might have raised Junio's eyebrows less to say something like:

   ...convert the existing strbuf_release() calls as well (they're not
   wrong, but they also accomplish nothing and create an inconsistency
   with the UNLEAKed variables).

That aside, the patch looks good.

Show 12 quoted lines

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3345a0d16..2daa4412a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1298,6 +1298,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		strbuf_release(&buf);
>  	}
>  
> +	UNLEAK(opts);
>  	if (opts.patch_mode || opts.pathspec.nr)
>  		return checkout_paths(&opts, new.name);
>  	else

Seeing hunks like this makes me happy with the UNLEAK() solution. It would have been a real pain to do this via actual freeing.

-Peff

Martin Ågren· Oct 2, 2017, 10:20 UTC · re: Jeff King · lore

Re: [PATCH] builtin/: add UNLEAKs

On 2 October 2017 at 08:25, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> On Sun, Oct 01, 2017 at 07:42:08PM +0200, Martin Ågren wrote:
>
>> Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the
>> variables in the same order as we've declared them. While addressing
>> `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as
>> well.
>
> It might have raised Junio's eyebrows less to say something like:
>
>    ...convert the existing strbuf_release() calls as well (they're not
>    wrong, but they also accomplish nothing and create an inconsistency
>    with the UNLEAKed variables).

Most likely yes. Sorry about that. I have yet to be critiqued for writing *too much* in a commit message. That should tell me something.

> Seeing hunks like this makes me happy with the UNLEAK() solution. It
> would have been a real pain to do this via actual freeing.

Yes, I was very happy to have it handy. :-)

Martin

Junio C Hamano· Oct 3, 2017, 06:20 UTC · re: Martin Ågren · lore

Re: [PATCH] builtin/: add UNLEAKs

Martin Ågren <martin.agren@gmail.com> writes:

>> Seeing hunks like this makes me happy with the UNLEAK() solution. It
>> would have been a real pain to do this via actual freeing.
>
> Yes, I was very happy to have it handy. :-)

OK, let's merge this to 'next', then.