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

> On Mon, Jun 23, 2014 at 11:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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.
>
> This approach has the added benefit of fixing the case where getenv
> uses a static buffer, like POSIX allows.