threads / patch / 52411

patchbisect--helper: avoid free-after-use

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

## tl;dr

5 messages between Dec 9, 2019 and Dec 9, 2019. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Miriam Rubio· Dec 9, 2019, 08:40 UTC · lore
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(-)
Show changes to builtin/bisect--helper.c +4 −3
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)
Johannes Schindelin· Dec 9, 2019, 09:33 UTC · re: Miriam Rubio · lore

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

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:
Show 16 quoted lines
> 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

Show 27 quoted lines
>  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)
>
>
Christian Couder· Dec 9, 2019, 10:22 UTC · re: Miriam Rubio · lore

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

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:
Show 11 quoted lines
>
> 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.

Show 5 quoted lines
> 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.

Miriam R.· Dec 9, 2019, 10:36 UTC · re: Christian Couder · lore

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

El lun., 9 dic. 2019 a las 11:22, Christian Couder (<christian.couder@gmail.com>) escribió:

Show 7 quoted lines
>
> 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.
Show 31 quoted lines
> 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.
Christian Couder· Dec 9, 2019, 11:10 UTC · re: Miriam R. · lore

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

On Mon, Dec 9, 2019 at 11:36 AM Miriam R. <mirucam@gmail.com> wrote:
Show 14 quoted lines
> > > 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.

← back to recent threads