Jeff King <peff@xxxxxxxx> writes:
> 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:
Looks quite straight-forward and readable, I would say. Not only you gave
us a legitimate excuse to get rid of the humourous messages, you lifted
most of the artificial limitations ('domainname' limit is still there but
that is not anything new) and use of strlcpy(), the last of which is a
huge win from my point of view ;-)
>
> ---
> 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);
--
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