[Outreachy] [PATCH] bisect--helper: avoid free-after-use

[Outreachy] [PATCH] bisect--helper: avoid free-after-use

[Miriam Rubio]

From: Tanushree Tumane <tanushreetumane@gmail.com>

In 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,
2019-01-02), the `git bisect reset` subcommand was ported to C. When the
call to `git checkout` failed, an error message was reported to the
user.

However, this error message used the `strbuf` that had just been
released already. Let's switch that around: first use it, then release
it.

Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1fbe156e67..3055b2bb50 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -169,11 +169,12 @@ static int bisect_reset(const char *commit)
 
 		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
 		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+			error(_("could not check out original"
+				" HEAD '%s'. Try 'git bisect"
+				" reset <commit>'."), branch.buf);
 			strbuf_release(&branch);
 			argv_array_clear(&argv);
-			return error(_("could not check out original"
-				       " HEAD '%s'. Try 'git bisect"
-				       " reset <commit>'."), branch.buf);
+			return -1;
 		}
 		argv_array_clear(&argv);
 	}
-- 
2.21.0 (Apple Git-122.2)

Re: [Outreachy] [PATCH] bisect--helper: avoid free-after-use

[Johannes Schindelin]

Hi Miriam,

just a little note on the process: the convention on the Git mailing list
is to use `[PATCH v2]`, `[PATCH v3]`, etc when sending revised patch
series. It is even available in `git format-patch`'s options: `-v 2` (see
https://git-scm.com/docs/git-format-patch#Documentation/git-format-patch.txt--vltngt)

On Mon, 9 Dec 2019, Miriam Rubio wrote:

> From: Tanushree Tumane <tanushreetumane@gmail.com>
>
> In 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,
> 2019-01-02), the `git bisect reset` subcommand was ported to C. When the
> call to `git checkout` failed, an error message was reported to the
> user.
>
> However, this error message used the `strbuf` that had just been
> released already. Let's switch that around: first use it, then release
> it.
>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---

ACK!

Thanks,
Dscho

>  builtin/bisect--helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1fbe156e67..3055b2bb50 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -169,11 +169,12 @@ static int bisect_reset(const char *commit)
>
>  		argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>  		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> +			error(_("could not check out original"
> +				" HEAD '%s'. Try 'git bisect"
> +				" reset <commit>'."), branch.buf);
>  			strbuf_release(&branch);
>  			argv_array_clear(&argv);
> -			return error(_("could not check out original"
> -				       " HEAD '%s'. Try 'git bisect"
> -				       " reset <commit>'."), branch.buf);
> +			return -1;
>  		}
>  		argv_array_clear(&argv);
>  	}
> --
> 2.21.0 (Apple Git-122.2)
>
>

Re: [Outreachy] [PATCH] bisect--helper: avoid free-after-use

[Christian Couder]

Hi Miriam,

As Dscho suggests, next time please use [PATCH v2] or [PATCH v3]
instead of [PATCH] if the patch has already been sent, even if the
subject has changed.

On Mon, Dec 9, 2019 at 9:40 AM Miriam Rubio <mirucam@gmail.com> wrote:
>
> From: Tanushree Tumane <tanushreetumane@gmail.com>
>
> In 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,
> 2019-01-02), the `git bisect reset` subcommand was ported to C. When the
> call to `git checkout` failed, an error message was reported to the
> user.
>
> However, this error message used the `strbuf` that had just been
> released already. Let's switch that around: first use it, then release
> it.

Great!

I think keeping Tanushree as the author and changing the commit
message and the subject was the right thing to do.

> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---

Here (after the line starting with "---") you can add comments about
the patch. One useful comment here would be to say that this patch is
a new version of
https://public-inbox.org/git/20191208172813.16518-1-mirucam@gmail.com/
which itself has been sent previously by Tanushree
(https://public-inbox.org/git/64117cde718f0d56ebfa4c30f4d8fe2155f5cf65.1551003074.git.gitgitgadget@gmail.com/).

Thanks,
Christian.

Re: [Outreachy] [PATCH] bisect--helper: avoid free-after-use

[Miriam R.]

El lun., 9 dic. 2019 a las 11:22, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Miriam,
>
> As Dscho suggests, next time please use [PATCH v2] or [PATCH v3]
> instead of [PATCH] if the patch has already been sent, even if the
> subject has changed.
>
Ok! I take note.

> On Mon, Dec 9, 2019 at 9:40 AM Miriam Rubio <mirucam@gmail.com> wrote:
> >
> > From: Tanushree Tumane <tanushreetumane@gmail.com>
> >
> > In 5e82c3dd22a (bisect--helper: `bisect_reset` shell function in C,
> > 2019-01-02), the `git bisect reset` subcommand was ported to C. When the
> > call to `git checkout` failed, an error message was reported to the
> > user.
> >
> > However, this error message used the `strbuf` that had just been
> > released already. Let's switch that around: first use it, then release
> > it.
>
> Great!
>
> I think keeping Tanushree as the author and changing the commit
> message and the subject was the right thing to do.
>
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > ---
>
> Here (after the line starting with "---") you can add comments about
> the patch. One useful comment here would be to say that this patch is
> a new version of
> https://public-inbox.org/git/20191208172813.16518-1-mirucam@gmail.com/
> which itself has been sent previously by Tanushree
> (https://public-inbox.org/git/64117cde718f0d56ebfa4c30f4d8fe2155f5cf65.1551003074.git.gitgitgadget@gmail.com/).
>
Ok, I'll resend another one (v2) adding this comments. :)
Thank you.

> Thanks,
> Christian.

Re: [Outreachy] [PATCH] bisect--helper: avoid free-after-use

[Christian Couder]

On Mon, Dec 9, 2019 at 11:36 AM Miriam R. <mirucam@gmail.com> wrote:

> > > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > > ---
> >
> > Here (after the line starting with "---") you can add comments about
> > the patch. One useful comment here would be to say that this patch is
> > a new version of
> > https://public-inbox.org/git/20191208172813.16518-1-mirucam@gmail.com/
> > which itself has been sent previously by Tanushree
> > (https://public-inbox.org/git/64117cde718f0d56ebfa4c30f4d8fe2155f5cf65.1551003074.git.gitgitgadget@gmail.com/).
> >
> Ok, I'll resend another one (v2) adding this comments. :)

It's not a big deal, but there was no need to resend another one, as
the comment is there only to help reviewers with extra information
related to where the patch comes from. It will not appear in the
commit that will be made from the patch (using `git am`) and I already
gave the information about where the patch comes from in my reply to
the patch.