Re: [PATCH 1/2] Change error messages in ident.c...

[Jeff King <peff@peff.net>]

On Thu, May 10, 2012 at 03:23:39PM -0400, Jeff King wrote:

> I am also tempted to suggest that we simply replace the static buffers
> with dynamic strbufs. I guess that may open up new vectors for an
> attacker to convince git to allocate arbitrary amounts of memory, but
> that is already pretty easy to do, so I doubt it's a big deal.

For reference, that patch would look like something like this:

---
 builtin/fmt-merge-msg.c | 14 ++++----
 cache.h                 |  5 ++-
 config.c                |  4 +--
 environment.c           |  4 +--
 http-push.c             |  2 +-
 ident.c                 | 94 ++++++++++++++++++-------------------------------
 6 files changed, 50 insertions(+), 73 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index a517f17..bb716c8 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -230,7 +230,8 @@ static void add_branch_desc(struct strbuf *out, const char *name)
 static void record_person(int which, struct string_list *people,
 			  struct commit *commit)
 {
-	char name_buf[MAX_GITNAME], *name, *name_end;
+	struct strbuf name_buf = STRBUF_INIT;
+	char *name, *name_end;
 	struct string_list_item *elem;
 	const char *field = (which == 'a') ? "\nauthor " : "\ncommitter ";
 
@@ -243,17 +244,18 @@ static void record_person(int which, struct string_list *people,
 		name_end--;
 	while (isspace(*name_end) && name <= name_end)
 		name_end--;
-	if (name_end < name || name + MAX_GITNAME <= name_end)
+	if (name_end < name)
 		return;
-	memcpy(name_buf, name, name_end - name + 1);
-	name_buf[name_end - name + 1] = '\0';
+	strbuf_add(&name_buf, name, name_end - name + 1);
 
-	elem = string_list_lookup(people, name_buf);
+	elem = string_list_lookup(people, name_buf.buf);
 	if (!elem) {
-		elem = string_list_insert(people, name_buf);
+		elem = string_list_insert(people, name_buf.buf);
 		elem->util = (void *)0;
 	}
 	elem->util = (void*)(util_as_integral(elem) + 1);
+
+	strbuf_release(&name_buf);
 }
 
 static int cmp_string_list_util_as_integral(const void *a_, const void *b_)
diff --git a/cache.h b/cache.h
index e14ffcd..0c1a332 100644
--- a/cache.h
+++ b/cache.h
@@ -1138,9 +1138,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define MAX_GITNAME (1000)
-extern char git_default_email[MAX_GITNAME];
-extern char git_default_name[MAX_GITNAME];
+extern struct strbuf git_default_email;
+extern struct strbuf git_default_name;
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
diff --git a/config.c b/config.c
index eeee986..69cb08c 100644
--- a/config.c
+++ b/config.c
@@ -767,7 +767,7 @@ static int git_default_user_config(const char *var, const char *value)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_name, value, sizeof(git_default_name));
+		strbuf_addstr(&git_default_name, value);
 		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
@@ -775,7 +775,7 @@ static int git_default_user_config(const char *var, const char *value)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strlcpy(git_default_email, value, sizeof(git_default_email));
+		strbuf_addstr(&git_default_email, value);
 		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
diff --git a/environment.c b/environment.c
index d7e6c65..f4e3b53 100644
--- a/environment.c
+++ b/environment.c
@@ -11,8 +11,8 @@
 #include "refs.h"
 #include "fmt-merge-msg.h"
 
-char git_default_email[MAX_GITNAME];
-char git_default_name[MAX_GITNAME];
+struct strbuf git_default_email = STRBUF_INIT;
+struct strbuf git_default_name = STRBUF_INIT;
 int user_ident_explicitly_given;
 int trust_executable_bit = 1;
 int trust_ctime = 1;
diff --git a/http-push.c b/http-push.c
index 1df7ab5..2362ffd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -904,7 +904,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	escaped = xml_entities(git_default_email);
+	escaped = xml_entities(git_default_email.buf);
 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
 	free(escaped);
 
diff --git a/ident.c b/ident.c
index 87c697c..c7bdb3f 100644
--- a/ident.c
+++ b/ident.c
@@ -15,42 +15,27 @@ static char git_default_date[50];
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
-static void copy_gecos(const struct passwd *w, char *name, size_t sz)
+static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
-	char *src, *dst;
-	size_t len, nlen;
-
-	nlen = strlen(w->pw_name);
+	char *src;
 
 	/* Traditionally GECOS field had office phone numbers etc, separated
 	 * with commas.  Also & stands for capitalized form of the login name.
 	 */
 
-	for (len = 0, dst = name, src = get_gecos(w); len < sz; src++) {
+	for (src = get_gecos(w); *src && *src != ','; src++) {
 		int ch = *src;
-		if (ch != '&') {
-			*dst++ = ch;
-			if (ch == 0 || ch == ',')
-				break;
-			len++;
-			continue;
-		}
-		if (len + nlen < sz) {
+		if (ch != '&')
+			strbuf_addch(name, ch);
+		else {
 			/* Sorry, Mr. McDonald... */
-			*dst++ = toupper(*w->pw_name);
-			memcpy(dst, w->pw_name + 1, nlen - 1);
-			dst += nlen - 1;
-			len += nlen;
+			strbuf_addch(name, toupper(*w->pw_name));
+			strbuf_addstr(name, w->pw_name + 1);
 		}
 	}
-	if (len < sz)
-		name[len] = 0;
-	else
-		die("Your parents must have hated you!");
-
 }
 
-static int add_mailname_host(char *buf, size_t len)
+static int add_mailname_host(struct strbuf *buf)
 {
 	FILE *mailname;
 
@@ -61,7 +46,7 @@ static int add_mailname_host(char *buf, size_t len)
 				strerror(errno));
 		return -1;
 	}
-	if (!fgets(buf, len, mailname)) {
+	if (strbuf_getline(buf, mailname, '\n') == EOF) {
 		if (ferror(mailname))
 			warning("cannot read /etc/mailname: %s",
 				strerror(errno));
@@ -73,48 +58,41 @@ static int add_mailname_host(char *buf, size_t len)
 	return 0;
 }
 
-static void add_domainname(char *buf, size_t len)
+static void add_domainname(struct strbuf *out)
 {
+	char buf[1024];
 	struct hostent *he;
-	size_t namelen;
 	const char *domainname;
 
-	if (gethostname(buf, len)) {
+	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 		return;
 	}
-	namelen = strlen(buf);
-	if (memchr(buf, '.', namelen))
+	strbuf_addstr(out, buf);
+	if (strchr(buf, '.'))
 		return;
 
 	he = gethostbyname(buf);
-	buf[namelen++] = '.';
-	buf += namelen;
-	len -= namelen;
+	strbuf_addch(out, '.');
 	if (he && (domainname = strchr(he->h_name, '.')))
-		strlcpy(buf, domainname + 1, len);
+		strbuf_addstr(out, domainname + 1);
 	else
-		strlcpy(buf, "(none)", len);
+		strbuf_addstr(out, "(none)");
 }
 
-static void copy_email(const struct passwd *pw)
+static void copy_email(const struct passwd *pw, struct strbuf *email)
 {
 	/*
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
 	 */
-	size_t len = strlen(pw->pw_name);
-	if (len > sizeof(git_default_email)/2)
-		die("Your sysadmin must hate you!");
-	memcpy(git_default_email, pw->pw_name, len);
-	git_default_email[len++] = '@';
-
-	if (!add_mailname_host(git_default_email + len,
-				sizeof(git_default_email) - len))
+	strbuf_addstr(email, pw->pw_name);
+	strbuf_addch(email, '@');
+
+	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(git_default_email + len,
-			sizeof(git_default_email) - len);
+	add_domainname(email);
 }
 
 static void setup_ident(const char **name, const char **emailp)
@@ -122,32 +100,31 @@ static void setup_ident(const char **name, const char **emailp)
 	struct passwd *pw = NULL;
 
 	/* Get the name ("gecos") */
-	if (!*name && !git_default_name[0]) {
+	if (!*name && !git_default_name.len) {
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		copy_gecos(pw, git_default_name, sizeof(git_default_name));
+		copy_gecos(pw, &git_default_name);
 	}
 	if (!*name)
-		*name = git_default_name;
+		*name = git_default_name.buf;
 
-	if (!*emailp && !git_default_email[0]) {
+	if (!*emailp && !git_default_email.len) {
 		const char *email = getenv("EMAIL");
 
 		if (email && email[0]) {
-			strlcpy(git_default_email, email,
-				sizeof(git_default_email));
+			strbuf_addstr(&git_default_email, email);
 			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else {
 			if (!pw)
 				pw = getpwuid(getuid());
 			if (!pw)
 				die("You don't exist. Go away!");
-			copy_email(pw);
+			copy_email(pw, &git_default_email);
 		}
 	}
 	if (!*emailp)
-		*emailp = git_default_email;
+		*emailp = git_default_email.buf;
 
 	/* And set the default date */
 	if (!git_default_date[0])
@@ -317,7 +294,7 @@ const char *fmt_ident(const char *name, const char *email,
 		struct passwd *pw;
 
 		if ((warn_on_no_name || error_on_no_name) &&
-		    name == git_default_name && env_hint) {
+		    name == git_default_name.buf && env_hint) {
 			fputs(env_hint, stderr);
 			env_hint = NULL; /* warn only once */
 		}
@@ -326,9 +303,8 @@ const char *fmt_ident(const char *name, const char *email,
 		pw = getpwuid(getuid());
 		if (!pw)
 			die("You don't exist. Go away!");
-		strlcpy(git_default_name, pw->pw_name,
-			sizeof(git_default_name));
-		name = git_default_name;
+		strbuf_addstr(&git_default_name, pw->pw_name);
+		name = git_default_name.buf;
 	}
 
 	strcpy(date, git_default_date);