[PATCH] builtin/: add UNLEAKs

[PATCH] builtin/: add UNLEAKs

[Martin Ågren]

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

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

Re: [PATCH] builtin/: add UNLEAKs

[Junio C Hamano]

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.

Re: [PATCH] builtin/: add UNLEAKs

[Jeff King]

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.

> 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

Re: [PATCH] builtin/: add UNLEAKs

[Martin Ågren]

On 2 October 2017 at 08:25, Jeff King <peff@peff.net> wrote:
> 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

Re: [PATCH] builtin/: add UNLEAKs

[Junio C Hamano]

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.