# [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond 23 messages from 2012-05-10 to 2012-05-15. Participants: Angus Hammond, Jeff King, Junio C Hamano, Nguyen Thai Ngoc Duy. Thread: https://gitlist.dev/t/30500 ## Angus Hammond, 2012-05-10 19:06 Subject: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond Message-ID: <1336676770-17965-1-git-send-email-angusgh@gmail.com> URL: https://gitlist.dev/e/1336676770-17965-1-git-send-email-angusgh%40gmail.com ``` --- ident.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ident.c b/ident.c index 87c697c..51a7a73 100644 --- a/ident.c +++ b/ident.c @@ -46,7 +46,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz) if (len < sz) name[len] = 0; else - die("Your parents must have hated you!"); + die("Your GECOS field is too long."); } @@ -106,7 +106,7 @@ static void copy_email(const struct passwd *pw) */ size_t len = strlen(pw->pw_name); if (len > sizeof(git_default_email)/2) - die("Your sysadmin must hate you!"); + die("Your name field in is too long."); memcpy(git_default_email, pw->pw_name, len); git_default_email[len++] = '@'; @@ -125,7 +125,7 @@ static void setup_ident(const char **name, const char **emailp) if (!*name && !git_default_name[0]) { pw = getpwuid(getuid()); if (!pw) - die("You don't exist. Go away!"); + die("Could not read your GECOS field."); copy_gecos(pw, git_default_name, sizeof(git_default_name)); } if (!*name) @@ -142,7 +142,7 @@ static void setup_ident(const char **name, const char **emailp) if (!pw) pw = getpwuid(getuid()); if (!pw) - die("You don't exist. Go away!"); + die("Could not read your GECOS field."); copy_email(pw); } } @@ -325,7 +325,7 @@ const char *fmt_ident(const char *name, const char *email, die("empty ident %s <%s> not allowed", name, email); pw = getpwuid(getuid()); if (!pw) - die("You don't exist. Go away!"); + die("Could not read your GECOS field."); strlcpy(git_default_name, pw->pw_name, sizeof(git_default_name)); name = git_default_name; -- 1.7.9.5 ``` ## Angus Hammond, 2012-05-10 19:06 Subject: [PATCH 2/2] Remove diagnostics section from commit-tree and var man pages New error messages shouldn't need explaining like the old ones did so just delete the diagnostics section of the man pages. Signed-off-by: Angus Hammond Message-ID: <1336676770-17965-2-git-send-email-angusgh@gmail.com> URL: https://gitlist.dev/e/1336676770-17965-2-git-send-email-angusgh%40gmail.com In-Reply-To: <1336676770-17965-1-git-send-email-angusgh@gmail.com> ``` --- Documentation/git-commit-tree.txt | 9 --------- Documentation/git-var.txt | 9 --------- 2 files changed, 18 deletions(-) diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt index cfb9906..eb8ee99 100644 --- a/Documentation/git-commit-tree.txt +++ b/Documentation/git-commit-tree.txt @@ -88,15 +88,6 @@ for one to be entered and terminated with ^D. include::date-formats.txt[] -Diagnostics ------------ -You don't exist. Go away!:: - The passwd(5) gecos field couldn't be read -Your parents must have hated you!:: - The passwd(5) gecos field is longer than a giant static buffer. -Your sysadmin must hate you!:: - The passwd(5) name field is longer than a giant static buffer. - Discussion ---------- diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index 988a323..67edf58 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -59,15 +59,6 @@ ifdef::git-default-pager[] The build you are using chose '{git-default-pager}' as the default. endif::git-default-pager[] -Diagnostics ------------ -You don't exist. Go away!:: - The passwd(5) gecos field couldn't be read -Your parents must have hated you!:: - The passwd(5) gecos field is longer than a giant static buffer. -Your sysadmin must hate you!:: - The passwd(5) name field is longer than a giant static buffer. - SEE ALSO -------- linkgit:git-commit-tree[1] -- 1.7.9.5 ``` ## Angus Hammond, 2012-05-10 19:21 Subject: Re: [PATCH 2/2] Remove diagnostics section from commit-tree and var man pages New error messages shouldn't need explaining like the old ones did so just delete the diagnostics section of the man pages. Signed-off-by: Angus Hammond Message-ID: URL: https://gitlist.dev/e/CAOBOgRb3d%2BoLLLYk6yU5-JUYXKth%2BUKguJ7gc-SX6wUcb5z1Fw%40mail.gmail.com In-Reply-To: <1336676770-17965-2-git-send-email-angusgh@gmail.com> ``` I have no idea how I managed to send this to myself and see it as one, properly formatted email despite it being 2 formatted diabolically with entire commits messages in the subjects. Sorry about that. These were meant to offer an alternative solution to the current issue over unusual error messages from commit-tree by bypassing the whole unix humour issue. They look it'll still be possible to apply them even if they are badly sent. If not and people think they're worth while I'd be happy (try and) send them again without this mess. Thanks Angus ``` ## Jeff King, 2012-05-10 19:23 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <20120510192339.GA32357@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120510192339.GA32357%40sigill.intra.peff.net In-Reply-To: <1336676770-17965-1-git-send-email-angusgh@gmail.com> ``` On Thu, May 10, 2012 at 08:06:09PM +0100, Angus Hammond wrote: > Subject: Re: [PATCH 1/2] Change error messages in ident.c Make error messages > caused by failed reads of the /etc/passwd file easier to understand. > Signed-off-by: Angus Hammond Holy line-breaks, Batman! As amusing as I find the existing messages, this is probably a good direction (although I find it unlikely that most people would see the messages under normal use). 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. > @@ -46,7 +46,7 @@ static void copy_gecos(const struct passwd *w, char *name, size_t sz) > if (len < sz) > name[len] = 0; > else > - die("Your parents must have hated you!"); > + die("Your GECOS field is too long."); I know that "GECOS" is the standard name for the field, but I wonder if it is a bit unnecessarily jargon-y. Wouldn't something like: die("unable to get real name from system password file: name too long"); be a little more friendly? It tells what operation we were actually performing, and it doesn't use any jargon. > @@ -106,7 +106,7 @@ static void copy_email(const struct passwd *pw) > */ > size_t len = strlen(pw->pw_name); > if (len > sizeof(git_default_email)/2) > - die("Your sysadmin must hate you!"); > + die("Your name field in is too long."); s/in is/is/. Also, similar complaints to above (if you see this message unexpectedly, you might ask "which name field? One inside a commit object?"). > [...] And similar comments for the rest of the messages. -Peff ``` ## Junio C Hamano, 2012-05-10 19:43 Subject: Re: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond Message-ID: <7vpqabn7o1.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpqabn7o1.fsf%40alter.siamese.dyndns.org In-Reply-To: <1336676770-17965-1-git-send-email-angusgh@gmail.com> ``` They are one of the oldest and humorous messages we have in the system, and more importantly, users will see them only once on a badly configured system. If there is no real-life reason (e.g. "if we do not change this message, Nuclear reactors will start misbehaving"), I would rather keep them as they are for hysterical raisins. But that is just my preference to keep Linus's twisted sense of humor. ``` ## Jeff King, 2012-05-10 19:56 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <20120510195646.GA18276@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120510195646.GA18276%40sigill.intra.peff.net In-Reply-To: <20120510192339.GA32357@sigill.intra.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); ``` ## Angus Hammond, 2012-05-10 19:57 Subject: Re: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond Message-ID: URL: https://gitlist.dev/e/CAOBOgRaAv%3DBoopuepHzBjDyMf-JVbmabwaGipczAtCjeUPtepw%40mail.gmail.com In-Reply-To: <7vpqabn7o1.fsf@alter.siamese.dyndns.org> ``` On 10 May 2012 20:43, Junio C Hamano wrote: > They are one of the oldest and humorous messages we have in the system, > and more importantly, users will see them only once on a badly configured > system.  If there is no real-life reason (e.g. "if we do not change this > message, Nuclear reactors will start misbehaving"), I would rather keep > them as they are for hysterical raisins. I'm not too worried either way, just tried to knock the patch out quickly because it came up and this seemed like the logical solution. In all honesty though, whilst I don't have a problem with unix humour being in git, I do have a bit of a problem with it being in error messages since when these are displayed it means that a users system is preventing them from using git for whatever reason, and at those times there's a good chance you're worried about fixing that problem, not laughing at a joke made by Linus several years ago. Just my 2 cents. It's probably not worth too much bother since it'll only ever show up very rarely. Thanks Angus ``` ## Junio C Hamano, 2012-05-10 20:04 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <7vipg3n6pu.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vipg3n6pu.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120510192339.GA32357@sigill.intra.peff.net> ``` Jeff King writes: > I am also tempted to suggest that we simply replace the static buffers > with dynamic strbufs. Yeah, I think that is a proper approach for this issue, as it will make two of these messages unnecessary (or all? I couldn't think of a way to deal with missing getpwent case myself, though). ``` ## Jeff King, 2012-05-10 20:22 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <20120510202227.GA30965@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120510202227.GA30965%40sigill.intra.peff.net In-Reply-To: <7vipg3n6pu.fsf@alter.siamese.dyndns.org> ``` On Thu, May 10, 2012 at 01:04:13PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I am also tempted to suggest that we simply replace the static buffers > > with dynamic strbufs. > > Yeah, I think that is a proper approach for this issue, as it will make > two of these messages unnecessary (or all? I couldn't think of a way > to deal with missing getpwent case myself, though). It doesn't get rid of the "you don't exist" message, and I think just dying there makes sense. But that is actually the one that I consider the most likely to happen in practice, and should probably have a more useful error message. -Peff ``` ## Junio C Hamano, 2012-05-10 20:28 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <7vehqrn5lm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vehqrn5lm.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120510202227.GA30965@sigill.intra.peff.net> ``` Jeff King writes: > It doesn't get rid of the "you don't exist" message, and I think just > dying there makes sense. But that is actually the one that I consider > the most likely to happen in practice, and should probably have a more > useful error message. Yeah, I do not think anybody minds losing that phrasing from that message (the "parents" and "sysadmin" were the humorous ones), and we certainly can phrase it differently, e.g. Your system didn't tell me your real name; hint: git help config and look for user.name or something. ``` ## Nguyen Thai Ngoc Duy, 2012-05-11 11:35 Subject: Re: [PATCH 1/2] Change error messages in ident.c Make error messages caused by failed reads of the /etc/passwd file easier to understand. Signed-off-by: Angus Hammond Message-ID: URL: https://gitlist.dev/e/CACsJy8AfrF8YyOA41F80igwG8DGfWyi%2BwRwpo6TvADe%3DFnZgag%40mail.gmail.com In-Reply-To: <1336676770-17965-1-git-send-email-angusgh@gmail.com> ``` On Fri, May 11, 2012 at 2:06 AM, Angus Hammond wrote: > --- >  ident.c |   10 +++++----- >  1 file changed, 5 insertions(+), 5 deletions(-) While you are touching this, perhaps you can also turn all die(xxx) in this file to die(_(xxx)), same for warning()? You touch 5 out of 11 already. And it helps make sure all the new strings are in the same humor level (aka none). _() allows the messages to be translated in another language, by the way. Also this on top so we get nice advice diff --git a/ident.c b/ident.c index 87c697c..b5a631f 100644 --- a/ident.c +++ b/ident.c @@ -289,7 +289,7 @@ person_only: } static const char *env_hint = -"\n" +N_("\n" "*** Please tell me who you are.\n" "\n" "Run\n" @@ -299,7 +299,7 @@ static const char *env_hint = "\n" "to set your account\'s default identity.\n" "Omit --global to set the identity only in this repository.\n" -"\n"; +"\n"); const char *fmt_ident(const char *name, const char *email, const char *date_str, int flag) @@ -318,7 +318,7 @@ const char *fmt_ident(const char *name, const char *email, if ((warn_on_no_name || error_on_no_name) && name == git_default_name && env_hint) { - fputs(env_hint, stderr); + fputs(_(env_hint), stderr); env_hint = NULL; /* warn only once */ } if (error_on_no_name) -- Duy ``` ## Junio C Hamano, 2012-05-11 22:53 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <7vehqqjpmw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vehqqjpmw.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120510195646.GA18276@sigill.intra.peff.net> ``` Jeff King 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); ``` ## Jeff King, 2012-05-11 23:13 Subject: Re: [PATCH 1/2] Change error messages in ident.c... Message-ID: <20120511231303.GA24611@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120511231303.GA24611%40sigill.intra.peff.net In-Reply-To: <7vehqqjpmw.fsf@alter.siamese.dyndns.org> ``` On Fri, May 11, 2012 at 03:53:43PM -0700, Junio C Hamano 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 ;-) Thanks. I'll re-roll with a commit message, and a follow-on patch to fix the "you don't exist" message. But probably tomorrow, as I am just finishing gitting for the day. -Peff ``` ## Jeff King, 2012-05-14 16:28 Subject: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120514162824.GA24457@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120514162824.GA24457%40sigill.intra.peff.net In-Reply-To: <20120511231303.GA24611@sigill.intra.peff.net> ``` When we pull the user's name from the GECOS field of the passwd file (or generate an email address based on their username and hostname), we put the result into a static buffer. While it's extremely unlikely that anybody ever hit these limits (after all, in such a case their parents must have hated them), we still had to deal with the error cases in our code. Converting these static buffers to strbufs lets us simplify the code and drop some error messages from the documentation that have confused some users. Note that there is still one length limitation: the gethostname interface requires us to provide a static buffer, so we arbitrarily choose 1024 bytes for the hostname. Signed-off-by: Jeff King --- I noticed in add_domainname that we look up the host via gethostname, and then if it is not fully qualified, call gethostbyname and steal the domain portion of the result, tacking it onto the hostname we got. That seems oddly complex to me, and like it could result in a bogus hostname if the unqualified name does not match the first part of the returned qualified name. E.g., if the /etc/hosts file contains something like: 192.168.1.1 foo.example.com bar.example.com bar (and your hostname is "bar"). I doubt it matters much in practice, and it is outside the scope of this patch, so I left it for now. Documentation/git-commit-tree.txt | 4 -- Documentation/git-var.txt | 4 -- builtin/fmt-merge-msg.c | 14 +++--- cache.h | 5 +-- config.c | 4 +- environment.c | 4 +- http-push.c | 2 +- ident.c | 94 +++++++++++++++------------------------ 8 files changed, 50 insertions(+), 81 deletions(-) diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt index cfb9906..eb12b2d 100644 --- a/Documentation/git-commit-tree.txt +++ b/Documentation/git-commit-tree.txt @@ -92,10 +92,6 @@ Diagnostics ----------- You don't exist. Go away!:: The passwd(5) gecos field couldn't be read -Your parents must have hated you!:: - The passwd(5) gecos field is longer than a giant static buffer. -Your sysadmin must hate you!:: - The passwd(5) name field is longer than a giant static buffer. Discussion ---------- diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index 988a323..3f703e3 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -63,10 +63,6 @@ Diagnostics ----------- You don't exist. Go away!:: The passwd(5) gecos field couldn't be read -Your parents must have hated you!:: - The passwd(5) gecos field is longer than a giant static buffer. -Your sysadmin must hate you!:: - The passwd(5) name field is longer than a giant static buffer. SEE ALSO -------- 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); -- 1.7.10.2.8.g1101eed ``` ## Jeff King, 2012-05-14 16:36 Subject: [PATCH 2/2] ident: report passwd errors with a more friendly message Message-ID: <20120514163622.GB24457@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120514163622.GB24457%40sigill.intra.peff.net In-Reply-To: <20120511231303.GA24611@sigill.intra.peff.net> ``` When getpwuid fails, we give a cute but cryptic message. While it makes sense if you know that getpwuid or identity functions are being called, this code is triggered behind the scenes by quite a few git commands these days (e.g., receive-pack on a remote server might use it for a reflog; the current message is hard to distinguish from an authentication error). Let's switch to something that gives a little more context. While we're at it, we can factor out all of the cut-and-pastes of the "you don't exist" message into a wrapper function. Rather than provide xgetpwuid, let's make it even more specific to just getting the passwd entry for the current uid. That's the only way we use getpwuid anyway, and it lets us make an even more specific error message. The current message also fails to mention errno. While the usual cause for getpwuid failing is that the user does not exist, mentioning errno makes it easier to diagnose these problems. Note that POSIX specifies that errno remain untouched if the passwd entry does not exist (but will be set on actual errors), whereas some systems will return ENOENT or similar for a missing entry. We handle both cases in our wrapper. Signed-off-by: Jeff King --- You earlier suggested to show a hint to set "user.name". That might be complicated by the fact that this message can come from a remote server. Or maybe since that is by far the minority case, we should disregard it and show the hint. I left it out of this patch, as it can be trivially added on top due to the refactoring. I also noticed that the version of getpwuid in compat/mingw.c completely disregards its uid argument. This isn't a problem in the current codebase, since we always feed getuid(). But since the new wrapper is explicitly about getting our _own_ pw entry, it might make more sense to convert our getpwuid() replacement into an xgetpwuid_self() replacement, which is slightly more accurate. I'll leave that cleanup to Johannes if he cares to do it. Documentation/git-commit-tree.txt | 5 ----- Documentation/git-var.txt | 5 ----- git-compat-util.h | 3 +++ ident.c | 12 +++--------- wrapper.c | 12 ++++++++++++ 5 files changed, 18 insertions(+), 19 deletions(-) diff --git a/Documentation/git-commit-tree.txt b/Documentation/git-commit-tree.txt index eb12b2d..eb8ee99 100644 --- a/Documentation/git-commit-tree.txt +++ b/Documentation/git-commit-tree.txt @@ -88,11 +88,6 @@ for one to be entered and terminated with ^D. include::date-formats.txt[] -Diagnostics ------------ -You don't exist. Go away!:: - The passwd(5) gecos field couldn't be read - Discussion ---------- diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index 3f703e3..67edf58 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -59,11 +59,6 @@ ifdef::git-default-pager[] The build you are using chose '{git-default-pager}' as the default. endif::git-default-pager[] -Diagnostics ------------ -You don't exist. Go away!:: - The passwd(5) gecos field couldn't be read - SEE ALSO -------- linkgit:git-commit-tree[1] diff --git a/git-compat-util.h b/git-compat-util.h index ed11ad8..5bd9ad7 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -595,4 +595,7 @@ int rmdir_or_warn(const char *path); */ int remove_or_warn(unsigned int mode, const char *path); +/* Get the passwd entry for the UID of the current process. */ +struct passwd *xgetpwuid_self(void); + #endif diff --git a/ident.c b/ident.c index c7bdb3f..72944ba 100644 --- a/ident.c +++ b/ident.c @@ -101,9 +101,7 @@ static void setup_ident(const char **name, const char **emailp) /* Get the name ("gecos") */ if (!*name && !git_default_name.len) { - pw = getpwuid(getuid()); - if (!pw) - die("You don't exist. Go away!"); + pw = xgetpwuid_self(); copy_gecos(pw, &git_default_name); } if (!*name) @@ -117,9 +115,7 @@ static void setup_ident(const char **name, const char **emailp) user_ident_explicitly_given |= IDENT_MAIL_GIVEN; } else { if (!pw) - pw = getpwuid(getuid()); - if (!pw) - die("You don't exist. Go away!"); + pw = xgetpwuid_self(); copy_email(pw, &git_default_email); } } @@ -300,9 +296,7 @@ const char *fmt_ident(const char *name, const char *email, } if (error_on_no_name) die("empty ident %s <%s> not allowed", name, email); - pw = getpwuid(getuid()); - if (!pw) - die("You don't exist. Go away!"); + pw = xgetpwuid_self(); strbuf_addstr(&git_default_name, pw->pw_name); name = git_default_name.buf; } diff --git a/wrapper.c b/wrapper.c index 6ccd059..b5e33e4 100644 --- a/wrapper.c +++ b/wrapper.c @@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file) { return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file); } + +struct passwd *xgetpwuid_self(void) +{ + struct passwd *pw; + + errno = 0; + pw = getpwuid(getuid()); + if (!pw) + die(_("unable to look up current user in the passwd file: %s"), + errno ? strerror(errno) : _("no such user")); + return pw; +} -- 1.7.10.2.8.g1101eed ``` ## Jeff King, 2012-05-14 17:05 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120514170533.GA29909@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120514170533.GA29909%40sigill.intra.peff.net In-Reply-To: <20120514162824.GA24457@sigill.intra.peff.net> ``` On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote: > I noticed in add_domainname that we look up the host via gethostname, > and then if it is not fully qualified, call gethostbyname and steal the > domain portion of the result, tacking it onto the hostname we got. > > That seems oddly complex to me, and like it could result in a bogus > hostname if the unqualified name does not match the first part of the > returned qualified name. E.g., if the /etc/hosts file contains something > like: > > 192.168.1.1 foo.example.com bar.example.com bar > > (and your hostname is "bar"). I doubt it matters much in practice, and > it is outside the scope of this patch, so I left it for now. It looks like a bug in adc3dbc (Use sensible domain name (the DNS one) when guessing ident information, 2005-10-21). Before that we used getdomainname, where that procedure made more sense. The patch below fixes it. I doubt it matters much in practice, but I think the resulting code is way less confusing to read. -- >8 -- Subject: [PATCH] ident: use full dns names to generate email addresses When we construct an email address from the username and hostname, we generate the host part of the email with this procedure: 1. add the result of gethostname 2. if it has a dot, ok, it's fully qualified 3. if not, then look up the unqualified hostname via gethostbyname; take the domain name of the result and append it to the hostname Step 3 can actually produce a bogus result, as the name returned by gethostbyname may not be related to the hostname we fed it (e.g., consider a machine "foo" with names "foo.one.example.com" and "bar.two.example.com"; we may have the latter returned and generate the bogus name "foo.two.example.com"). This patch simply uses the full hostname returned by gethostbyname. In the common case that the first part is the same as the unqualified hostname, the behavior is identical. And in the case that it is not the same, we are much more likely to be generating a valid name. Signed-off-by: Jeff King --- ident.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/ident.c b/ident.c index 72944ba..e552e7f 100644 --- a/ident.c +++ b/ident.c @@ -62,23 +62,18 @@ static void add_domainname(struct strbuf *out) { char buf[1024]; struct hostent *he; - const char *domainname; if (gethostname(buf, sizeof(buf))) { warning("cannot get host name: %s", strerror(errno)); strbuf_addstr(out, "(none)"); return; } - strbuf_addstr(out, buf); if (strchr(buf, '.')) - return; - - he = gethostbyname(buf); - strbuf_addch(out, '.'); - if (he && (domainname = strchr(he->h_name, '.'))) - strbuf_addstr(out, domainname + 1); + strbuf_addstr(out, buf); + else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.')) + strbuf_addstr(out, he->h_name); else - strbuf_addstr(out, "(none)"); + strbuf_addf(out, "%s.(none)", buf); } static void copy_email(const struct passwd *pw, struct strbuf *email) -- 1.7.10.2.8.g1101eed ``` ## Jeff King, 2012-05-14 21:02 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120514210225.GA9677@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120514210225.GA9677%40sigill.intra.peff.net In-Reply-To: <20120514162824.GA24457@sigill.intra.peff.net> ``` On Mon, May 14, 2012 at 12:28:24PM -0400, Jeff King wrote: > When we pull the user's name from the GECOS field of the > passwd file (or generate an email address based on their > username and hostname), we put the result into a > static buffer. While it's extremely unlikely that anybody > ever hit these limits (after all, in such a case their > parents must have hated them), we still had to deal with the > error cases in our code. > > Converting these static buffers to strbufs lets us simplify > the code and drop some error messages from the documentation > that have confused some users. > > Note that there is still one length limitation: the > gethostname interface requires us to provide a static > buffer, so we arbitrarily choose 1024 bytes for the > hostname. > > Signed-off-by: Jeff King Ick, there is something very wrong with this patch. While testing a completely unrelated bug, I noticed that it set my name to "Jeff KingJeff KingJeff King". Which, while a wonderful ego massage, is probably excessive. I'm sure the problem is the switch to strbuf's appending semantics rather than strlcpy's overwriting semantics. I thought we were careful not to bother re-run the gecos code if we had already gotten a name, but obviously that is not the case in some code paths. I'll investigate. -Peff ``` ## Jeff King, 2012-05-14 21:13 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120514211324.GA11578@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120514211324.GA11578%40sigill.intra.peff.net In-Reply-To: <20120514210225.GA9677@sigill.intra.peff.net> ``` On Mon, May 14, 2012 at 05:02:25PM -0400, Jeff King wrote: > Ick, there is something very wrong with this patch. While testing a > completely unrelated bug, I noticed that it set my name to "Jeff > KingJeff KingJeff King". Which, while a wonderful ego massage, is > probably excessive. > > I'm sure the problem is the switch to strbuf's appending semantics > rather than strlcpy's overwriting semantics. I thought we were careful > not to bother re-run the gecos code if we had already gotten a name, but > obviously that is not the case in some code paths. I'll investigate. Ah, I see. The problem is here: > --- 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; where we are not careful. The fix is trivial. However, while examining fmt_ident, I notice there is another potential spot there that needs further investigation (I think it may actually be unreachable code, but I need to look closer). I'll re-roll the series with the fixes after investigating fmt_ident. -Peff ``` ## Jeff King, 2012-05-15 01:54 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120515015437.GA13833@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120515015437.GA13833%40sigill.intra.peff.net In-Reply-To: <20120514211324.GA11578@sigill.intra.peff.net> ``` On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote: > where we are not careful. The fix is trivial. However, while examining > fmt_ident, I notice there is another potential spot there that needs > further investigation (I think it may actually be unreachable code, but > I need to look closer). > > I'll re-roll the series with the fixes after investigating fmt_ident. Hmm. This code from fmt_ident is very odd: > const char *fmt_ident(const char *name, const char *email, > const char *date_str, int flag) > { > [...] > setup_ident(&name, &email); > > if (!*name) { > struct passwd *pw; > > if ((warn_on_no_name || error_on_no_name) && > name == git_default_name && env_hint) { > fputs(env_hint, stderr); > env_hint = NULL; /* warn only once */ > } > if (error_on_no_name) > die("empty ident %s <%s> not allowed", name, 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; > } We call setup_ident with our name pointer, which usually comes from getenv("GIT_*_NAME"), although could also come from something like "git commit -c $commit". We feed that to setup_ident. If name is NULL, then setup_ident will use git_default_name (filling it in from gecos or config). If it's not NULL, then we use it literally. And then we check _that_ result to see if it's empty. If it is, we either die or warn, depending on the flags. In the latter case, we fallback to using the username as the name. And that's what confuses me. Depending on what was passed in, we may have checked that GIT_COMMITTER_NAME is an empty string, or we may have checked that the config or gecos field yielded an empty string. In the latter case, it makes sense to fall back to the username. But in the former case, it doesn't; we should fall back to the config name or the gecos name. And worse, we've polluted git_default_name for the rest of the program run. Instead of falling back to getpwuid(), should it fall back to: /* If this wasn't our default name already, then fall back to that. */ if (name != git_default_name) { name = NULL; setup_ident(&name, &email); } /* If we _still_ don't have a non-empty name, then fall back to * username. */ if (!*name) { pw = getpwuid(getuid()); if (!pw) die("You don't exist. Go away!"); strlcpy(git_default_name, pw->pw_name, sizeof(git_default_name)); nae = git_default_name; } Of course we've still polluted this crappy fake name into git_default_name, so that later calls with error_on_no_name will see it and not error. I think so far it hasn't mattered because the only user of this "warn" code is format-patch, which otherwise does not care about ident (and doesn't even end up using the name at all!). And I doubt this code path gets triggered much anyway; do people really run "GIT_COMMITTER_NAME= git format-patch"? I can just leave it as it's not really hurting anybody, I think. But I was refactoring in the area and it just seemed flaky and questionable. I wonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path entirely. The use here is grabbing the email address to use as part of a message id. Could we just call setup_ident and then read from git_default_email directly? There's no need to respect GIT_COMMITTER_EMAIL here at all. -Peff ``` ## Jeff King, 2012-05-15 02:32 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120515023220.GA22947@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120515023220.GA22947%40sigill.intra.peff.net In-Reply-To: <20120515015437.GA13833@sigill.intra.peff.net> ``` On Mon, May 14, 2012 at 09:54:37PM -0400, Jeff King wrote: > Of course we've still polluted this crappy fake name into > git_default_name, so that later calls with error_on_no_name will see it > and not error. I think so far it hasn't mattered because the only user > of this "warn" code is format-patch, which otherwise does not care about > ident (and doesn't even end up using the name at all!). And I doubt this > code path gets triggered much anyway; do people really run > "GIT_COMMITTER_NAME= git format-patch"? > > I can just leave it as it's not really hurting anybody, I think. But I > was refactoring in the area and it just seemed flaky and questionable. I > wonder if we can simply get rid of the IDENT_WARN_ON_NO_NAME code path > entirely. The use here is grabbing the email address to use as part of a > message id. Could we just call setup_ident and then read from > git_default_email directly? There's no need to respect > GIT_COMMITTER_EMAIL here at all. Hmm, I was mistaken. This code path also gets followed whenever IDENT_ERROR_ON_NO_NAME is not set (regardless of IDENT_WARN_ON_NO_NAME). So other programs may accidentally get this pollution of git_default_name and show a username when we _could_ have shown the name from config. I can see the pollution in a debugger in "git commit", but I don't think you can actually trigger a commit with it, because later calls to fmt_ident use ERROR_ON_NO_NAME. I really wonder if we can just get rid of all of the calls which do not use ERROR_ON_NO_NAME. As far as I can tell, they are all part of programs which later end up using ERROR_ON_NO_NAME anyway. -Peff ``` ## Junio C Hamano, 2012-05-15 15:03 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <7vtxzhfpv9.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtxzhfpv9.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120515015437.GA13833@sigill.intra.peff.net> ``` Jeff King writes: > On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote: > > We call setup_ident with our name pointer, which usually comes from > getenv("GIT_*_NAME"), although could also come from something like "git > commit -c $commit". We feed that to setup_ident. If name is NULL, then > setup_ident will use git_default_name (filling it in from gecos or > config). If it's not NULL, then we use it literally. And then we check > _that_ result to see if it's empty. If it is, we either die or warn, > depending on the flags. In the latter case, we fallback to using the > username as the name. > > And that's what confuses me. Depending on what was passed in, we may > have checked that GIT_COMMITTER_NAME is an empty string, or we may have > checked that the config or gecos field yielded an empty string. Sounds quite sensible to me, though. > In the > latter case, it makes sense to fall back to the username. I agree that we should use something like "Sorry, Mr. McDonald" codepath when the GECOS field returns an empty string---after all that is what we do when we are built with NO_GECOS_IN_PWENT. > But in the > former case, it doesn't; we should fall back to the config name or the > gecos name. If the user said GIT_COMMITTER_NAME is empty with "GIT_COMMITTER_NAME=", that is different from saying with "unset GIT_COMMITTER_NAME" that the user does not want the environment to take effect, no? So I do not think falling back to configured or gecos in the former case is the right thing to do, even though that would mean explicitly giving an empty string in that configuration variable is asking only for an error without any recourse, which is not useful at all. ``` ## Jeff King, 2012-05-15 17:47 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <20120515174724.GA329@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120515174724.GA329%40sigill.intra.peff.net In-Reply-To: <7vtxzhfpv9.fsf@alter.siamese.dyndns.org> ``` On Tue, May 15, 2012 at 08:03:38AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote: > > > > We call setup_ident with our name pointer, which usually comes from > > getenv("GIT_*_NAME"), although could also come from something like "git > > commit -c $commit". We feed that to setup_ident. If name is NULL, then > > setup_ident will use git_default_name (filling it in from gecos or > > config). If it's not NULL, then we use it literally. And then we check > > _that_ result to see if it's empty. If it is, we either die or warn, > > depending on the flags. In the latter case, we fallback to using the > > username as the name. > > > > And that's what confuses me. Depending on what was passed in, we may > > have checked that GIT_COMMITTER_NAME is an empty string, or we may have > > checked that the config or gecos field yielded an empty string. > > Sounds quite sensible to me, though. Yes, I think it is OK to check what was given to us (or our fallback). But using that check to decide which next step to take doesn't seem right. > > In the > > latter case, it makes sense to fall back to the username. > > I agree that we should use something like "Sorry, Mr. McDonald" codepath > when the GECOS field returns an empty string---after all that is what we > do when we are built with NO_GECOS_IN_PWENT. Right, and that is more or less what we do (just without the capitalization). > > But in the > > former case, it doesn't; we should fall back to the config name or the > > gecos name. > > If the user said GIT_COMMITTER_NAME is empty with "GIT_COMMITTER_NAME=", > that is different from saying with "unset GIT_COMMITTER_NAME" that the > user does not want the environment to take effect, no? I agree two the cases are different. And for the most part, you are insane to pass an empty GIT_COMMITTER_NAME. But if you do, why would the right behavior be to fall back to sticking the username into the name field, and not the gecos name? Part of me is wondering why we should fall back at all in that case. If a caller does not pass ERROR_ON_NO_NAME, then they don't really care what the name is, do they? The current callers that do not pass it are: - blame.c:fake_working_tree_commit, which is passing in a fake name buffer anyway (so will never trigger this code path) - log.c:gen_message_id, which only cares about the email portion anyway - fmt-merge-msg.c:credit_people; this caller compares the name field to what's in the commits, checking for differences. So it could just as easily be "(none)" or some other token - commit.c:prepare_to_commit; this compares and shows author and commiter ids, and does not care about a blank name for the committer (but does for the author). The commit can't go through anyway with a blank committer name, so should it not just use ERROR_ON_NO_NAME? - log.c:make_cover_letter; this uses the committer information to make a fake commit that we ultimately use just to get the "%f" pretty userformat from it. In other words, we don't care about the committer at all, and this is really just working around an absolutely horrific interface. - refs.c:log_ref_write; finally, a caller who actually cares about the name, but doesn't want to die if we don't have a good name. We are happy enough with the username, though if somebody passes GIT_COMMITTER_NAME=, wouldn't it be OK to fail? So it seems to me like a much simpler set of rules would be: 1. When reading gecos, always fall back to the username if the gecos field is unavailable or blank. 2. Always die when the name field is blank. That means we will die when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty config name), which makes a lot more sense to me than falling back; those are bogus requests, not system config problems. And we won't ever have a blank gecos name, because we'll always fall back on the username. Again, I'm sorry to belabor this, and we can just drop it; I don't think there's currently a bug. It's just that I'm cleaning up in the area, and the current behavior seems overly complex; in particular, I'm worried that writing the username into the git_default_name field (overwriting the _real_ name the user gave us!) is a maintenance time-bomb that will bite us later. If I'm not being clear, I can express it in the form of patches, which might be more obvious. -Peff ``` ## Junio C Hamano, 2012-05-15 18:10 Subject: Re: [PATCH 1/2] drop length limitations on gecos-derived names and emails Message-ID: <7vsjf1e2n7.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjf1e2n7.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120515174724.GA329@sigill.intra.peff.net> ``` Jeff King writes: > So it seems to me like a much simpler set of rules would be: > > 1. When reading gecos, always fall back to the username if the gecos > field is unavailable or blank. > > 2. Always die when the name field is blank. That means we will die > when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty > config name), which makes a lot more sense to me than falling back; > those are bogus requests, not system config problems. And we won't > ever have a blank gecos name, because we'll always fall back on the > username. That certainly sounds very simple to explain and understand, and I do not offhand think of anything *sane* that would break ;-) Thanks. ```