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

[Jeff King <peff@peff.net>]

On Mon, Jun 23, 2014 at 05:28:14AM -0400, Eric Sunshine wrote:

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

Yeah, you wouldn't believe the number of times I switched back and forth
while writing the patch. The variable is called "mail" in "struct
ident_split", so you have a mismatch at some point unless we rename the
local pointer.

I avoided doing that to keep the diff smaller, but perhaps it's worth
doing.

> >                 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()?

Yeah, I agree it is getting a bit complicated (I tried to introduce
helpers like set_pair to at least keep the per-variable work down to a
single line in each instance).

It unfortunately doesn't work to just pass the ".buf" of each strbuf. We
care about the distinction between the empty string and NULL here (the
latter will cause fmt_ident to use the default 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.

Yeah, I also considered that. You end up having to free() before
re-assigning throughout the function, though that is not much worse than
having to strbuf_reset() before re-adding (the reset is hidden in
set_pair, but we could similarly abstract it).

Here's what that looks like (this converts date_buf away, too, to avoid
relying on getenv()'s static return value):

diff --git a/builtin/commit.c b/builtin/commit.c
index 62abee0..35045ca 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -551,15 +551,26 @@ static char *xmemdupz_pair(const struct pointer_pair *p)
 	return xmemdupz(p->begin, p->end - p->begin);
 }
 
+static void set_ident_var(char **buf, char *val)
+{
+	free(*buf);
+	*buf = val;
+}
+
+static char *envdup(const char *var)
+{
+	const char *val = getenv(var);
+	return val ? xstrdup(val) : NULL;
+}
+
 static void determine_author_info(struct strbuf *author_ident)
 {
 	char *name, *email, *date;
 	struct ident_split author;
-	struct strbuf date_buf = STRBUF_INIT;
 
-	name = getenv("GIT_AUTHOR_NAME");
-	email = getenv("GIT_AUTHOR_EMAIL");
-	date = getenv("GIT_AUTHOR_DATE");
+	name = envdup("GIT_AUTHOR_NAME");
+	email = envdup("GIT_AUTHOR_EMAIL");
+	date = envdup("GIT_AUTHOR_DATE");
 
 	if (author_message) {
 		struct ident_split ident;
@@ -572,15 +583,15 @@ 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);
+		set_ident_var(&name, xmemdupz_pair(&ident.name));
+		set_ident_var(&email, xmemdupz_pair(&ident.mail));
 		if (ident.date.begin) {
-			strbuf_reset(&date_buf);
+			struct strbuf date_buf = STRBUF_INIT;
 			strbuf_addch(&date_buf, '@');
 			strbuf_add_pair(&date_buf, &ident.date);
 			strbuf_addch(&date_buf, ' ');
 			strbuf_add_pair(&date_buf, &ident.tz);
-			date = date_buf.buf;
+			set_ident_var(&date, strbuf_detach(&date_buf, NULL));
 		}
 	}
 
@@ -589,15 +600,15 @@ 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);
+		set_ident_var(&name, xmemdupz_pair(&ident.name));
+		set_ident_var(&email, xmemdupz_pair(&ident.mail));
 	}
 
 	if (force_date) {
-		strbuf_reset(&date_buf);
+		struct strbuf date_buf = STRBUF_INIT;
 		if (parse_force_date(force_date, &date_buf))
 			die(_("invalid date format: %s"), force_date);
-		date = date_buf.buf;
+		set_ident_var(&date, strbuf_detach(&date_buf, NULL));
 	}
 
 	strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
@@ -608,7 +619,9 @@ static void determine_author_info(struct strbuf *author_ident)
 		export_one("GIT_AUTHOR_DATE", author.date.begin, author.tz.end, '@');
 	}
 
-	strbuf_release(&date_buf);
+	free(name);
+	free(email);
+	free(date);
 }
 
 static void split_ident_or_die(struct ident_split *id, const struct strbuf *buf)


I dunno. Maybe the set_ident_var helper is a little too cutesy and
obfuscating.

-Peff