On Mon, Jun 23, 2014 at 5:33 AM, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote:
> On Mon, Jun 23, 2014 at 11:28 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Wed, Jun 18, 2014 at 4:36 PM, Jeff King <peff@xxxxxxxx> 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@xxxxxxxx>
>>> ---
>>> 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.
That reminds me that I was going to suggest a suitable variation of
envdup if Peff wanted to keep the strbufs:
static void strbuf_env(struct strbuf *s, const char *e)
{
const char *v = getenv(e);
if (v)
strbuf_add(s, v);
}
(Or add a generalized strbuf_add_gently() to strbuf.{h,c}, if it seems
likely to come up more often, which would allow
strbuf_add_gently(&name_buf, getenv("GIT_AUTHOR_NAME")) -- with a
better name than "gently", of course.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html