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

> On Wed, Jun 18, 2014 at 4:36 PM, Jeff King <peff@peff.net> wrote:
>> 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
>
>> +                     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.