git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Jun 23, 2014, 17:20 UTC
Message-ID
<20140623172025.GB4838@sigill.intra.peff.net>
In-Reply-To
<CAPig+cT7mAGaGXZHNEWvZ31acth2wooexZ5s7wWFrJ40rBviYw@mail.gmail.com>
On Mon, Jun 23, 2014 at 05:28:14AM -0400, Eric Sunshine wrote:
Show 10 quoted lines
> >  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.

Show 11 quoted lines
> >                 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).

Show 14 quoted lines
> 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
Previous: Jeff King
Message 16 of 16 in “cleaning up determine_author_info”
  1. 0/7 cleaning up determine_author_infoJeff King, Jun 18, 2014
  2. 1/7 commit: provide a function to find a header in a bufferJeff King, Jun 18, 2014
  3. Eric SunshineJun 23, 2014
  4. Jeff KingJun 23, 2014
  5. 2/7 record_author_info: fix memory leak on malformed commitJeff King, Jun 18, 2014
  6. 3/7 record_author_info: use find_commit_headerJeff King, Jun 18, 2014
  7. 4/7 ident_split: store begin/end pairs on their own structJeff King, Jun 18, 2014
  8. Eric SunshineJun 23, 2014
  9. 5/7 use strbufs in date functionsJeff King, Jun 18, 2014
  10. 6/7 determine_author_info: reuse parsing functionsJeff King, Jun 18, 2014
  11. 7/7 determine_author_info: stop leaking name/emailJeff King, Jun 18, 2014
  12. Eric SunshineJun 23, 2014
  13. Erik Faye-LundJun 23, 2014
  14. Eric SunshineJun 23, 2014
  15. Jeff KingJun 23, 2014
  16. Jeff KingJun 23, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.