Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> @@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  	} else if (edit_description) {
>  		const char *branch_name;
>  		struct strbuf branch_ref = STRBUF_INIT;
> +		struct strbuf buf = STRBUF_INIT;
>  
>  		if (!argc) {
>  			if (filter.detached)
>  				die(_("Cannot give description to detached HEAD"));
>  			branch_name = head;
> -		} else if (argc == 1)
> -			branch_name = argv[0];
> +		} else if (argc == 1) {
> +			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> +			branch_name = buf.buf;
> +		}
>  		else
>  			die(_("cannot edit description of more than one branch"));

Here branch_name could be pointing at buf.buf (or head).

>  		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
>  		if (!ref_exists(branch_ref.buf)) {
>  			strbuf_release(&branch_ref);
> +			strbuf_release(&buf);

But the contents of the buf becomes invalid at this point.

>  			if (!argc)
>  				return error(_("No commit on branch '%s' yet."),

In the post context of this hunk, we see that the '%s' above is
filled with branch_name, accessing the potentially invalid contents.

I'll put a squashable band-aid on top of the topic for now.

--- >8 ---

 * cmd_foo() should not return an negative value.

 * branch_name used in the calls to error() could point at buf.buf
   that holds the expansion of @{-1}, but buf was released way too
   early, leading to a use-after-free.

 * Style: if/else if/else cascade whose one arm has multiple
   statements and requires {braces} around it should have {braces}
   around all of its arms.

 * each arm in the top-level if/else if/else cascade for "git
   branch" subcommands were more or less independent, and there
   wasn't anything common that they need to execute after exiting
   the cascade.  Unconditionally returning from the arm for the
   edit-description subcommand seems to make the logic flow easier
   to read.

 builtin/branch.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 197603241d..17853225fa 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -792,6 +792,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		const char *branch_name;
 		struct strbuf branch_ref = STRBUF_INIT;
 		struct strbuf buf = STRBUF_INIT;
+		int ret = 1; /* assume failure */
 
 		if (!argc) {
 			if (filter.detached)
@@ -800,29 +801,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		} else if (argc == 1) {
 			strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
 			branch_name = buf.buf;
-		}
-		else
+		} else {
 			die(_("cannot edit description of more than one branch"));
+		}
 
 		strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
-		if (!ref_exists(branch_ref.buf)) {
-			strbuf_release(&branch_ref);
-			strbuf_release(&buf);
-
-			if (!argc)
-				return error(_("No commit on branch '%s' yet."),
-					     branch_name);
-			else
-				return error(_("No branch named '%s'."),
-					     branch_name);
-		}
-		strbuf_release(&branch_ref);
+		if (!ref_exists(branch_ref.buf))
+			error(!argc
+			      ? _("No commit on branch '%s' yet.")
+			      : _("No branch named '%s'."),
+			      branch_name);
+		else if (!edit_branch_description(branch_name))
+			ret = 0; /* happy */
 
-		if (edit_branch_description(branch_name)) {
-			strbuf_release(&buf);
-			return 1;
-		}
+		strbuf_release(&branch_ref);
 		strbuf_release(&buf);
+
+		return ret;
 	} else if (copy) {
 		if (!argc)
 			die(_("branch name required"));
-- 
2.38.0-167-g3030fd6006