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.
--
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