Re: [PATCH 7/7] determine_author_info: stop leaking name/email

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 23, 2014, 09:28 UTC
Message-ID: <CAPig+cT7mAGaGXZHNEWvZ31acth2wooexZ5s7wWFrJ40rBviYw@mail.gmail.com>
In-Reply-To: <20140618203609.GG23896@sigill.intra.peff.net>

On Wed, Jun 18, 2014 at 4:36 PM, Jeff King <peff@peff.net> wrote:

Show 39 quoted lines

> When we get the author name and email either from an
> existing commit or from the "--author" option, we create a
> copy of the strings. We cannot just free() these copies,
> since the same pointers may also be pointing to getenv()
> storage which we do not own.
>
> Instead, let's treat these the same way as we do the date
> buffer: keep a strbuf to be released, and point the bare
> pointers at the strbuf.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/commit.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 62abee0..72beb7f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)
>         strbuf_add(buf, p->begin, p->end - p->begin);
>  }
>
> -static char *xmemdupz_pair(const struct pointer_pair *p)
> +static char *set_pair(struct strbuf *buf, struct pointer_pair *p)
>  {
> -       return xmemdupz(p->begin, p->end - p->begin);
> +       strbuf_reset(buf);
> +       strbuf_add_pair(buf, p);
> +       return buf->buf;
>  }
>
>  static void determine_author_info(struct strbuf *author_ident)
>  {
>         char *name, *email, *date;
>         struct ident_split author;
> -       struct strbuf date_buf = STRBUF_INIT;
> +       struct strbuf name_buf = STRBUF_INIT,
> +                     mail_buf = STRBUF_INIT,

nit: The associated 'char *' variable is named "email", so perhaps s/mail_buf/email_buf/g

Show 23 quoted lines

> +                     date_buf = STRBUF_INIT;
>
>         name = getenv("GIT_AUTHOR_NAME");
>         email = getenv("GIT_AUTHOR_EMAIL");
> @@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf *author_ident)
>                 if (split_ident_line(&ident, a, len) < 0)
>                         die(_("commit '%s' has malformed author line"), author_message);
>
> -               name = xmemdupz_pair(&ident.name);
> -               email = xmemdupz_pair(&ident.mail);
> +               name = set_pair(&name_buf, &ident.name);
> +               email = set_pair(&mail_buf, &ident.mail);
>                 if (ident.date.begin) {
>                         strbuf_reset(&date_buf);
>                         strbuf_addch(&date_buf, '@');
> @@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf *author_ident)
>
>                 if (split_ident_line(&ident, force_author, strlen(force_author)) < 0)
>                         die(_("malformed --author parameter"));
> -               name = xmemdupz_pair(&ident.name);
> -               email = xmemdupz_pair(&ident.mail);
> +               name = set_pair(&name_buf, &ident.name);
> +               email = set_pair(&mail_buf, &ident.mail);

Does the code become too convoluted with these changes? You're now maintaining three 'char *' variables in parallel with three strbuf variables. Is it possible to drop the 'char *' variables and just pass the .buf member of the strbufs to fmt_ident()?

Alternately, you also could solve the leaks by having an envdup() helper:

    static char *envdup(const char *s)
    {
        const char *v = getenv(s);
        return v ? xstrdup(v) : NULL;
    }

    ...
    name = envdup("GIT_AUTHOR_NAME");
    email = envdup("GIT_AUTHOR_EMAIL");
    ...

And then just free() 'name' and 'email' normally.

Show 14 quoted lines

>         }
>
>         if (force_date) {
> @@ -608,6 +612,8 @@ static void determine_author_info(struct strbuf *author_ident)
>                 export_one("GIT_AUTHOR_DATE", author.date.begin, author.tz.end, '@');
>         }
>
> +       strbuf_release(&name_buf);
> +       strbuf_release(&mail_buf);
>         strbuf_release(&date_buf);
>  }
>
> --
> 2.0.0.566.gfe3e6b2