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