| Message ID | 271a5f57cef00d24ee33e99839cf3890e561efe7.1539596822.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Provide a useful default user ident on Windows | expand |
On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > When a user is registered in a Windows domain, it is really easy to > obtain the email address. So let's do that. > [...] > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > diff --git a/compat/mingw.c b/compat/mingw.c > @@ -1826,6 +1826,11 @@ static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) > +char *mingw_query_user_email(void) > +{ > + return get_extended_user_info(NameUserPrincipal); > +} > diff --git a/ident.c b/ident.c > @@ -168,6 +168,9 @@ const char *ident_default_email(void) > + } else if ((email = query_user_email()) && email[0]) { > + strbuf_addstr(&git_default_email, email); > + free((char *)email); If query_user_email(), which calls get_extended_user_info(), returns NULL, then we do nothing (and don't worry about freeing the result). However, if query_user_email() returns a zero-length allocated string, then this conditional will leak that string (due to the email[0] check). From patch 2/3, we see in get_extended_user_info(): +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) +{ + if (GetUserNameExW(type, wbuffer, &len)) { + char *converted = xmalloc((len *= 3)); + if (xwcstoutf(converted, wbuffer, len) >= 0) + return converted; that it may possibly return a zero-length string (due to the ">=0"). Do we care about this potential leak (or is GetUserNameExW() guaranteed never to return such a string)?
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > +char *mingw_query_user_email(void) > +{ > + return get_extended_user_info(NameUserPrincipal); > +} > + > ... > > +#ifndef query_user_email > +#define query_user_email() NULL > +#endif The three patches look sensible to me; will queue. You may already have audited our use of "struct passwd", "getpwnam()" and "getpwuid()"--I haven't. I think we use these only to learn user's email (to be used as the default ident) and home directory (to expand "git config --type=path"). If that is really the case, it may be a worthwhile clean-up to introduce our own API that offers these two exact functions, have the per-platform implementation of the API in compat/, and get rid of "struct passwd" and calls to getpw*() functions out of the core Git proper, to wean ourselves away from depending on POSIX too much. But of course that is a separate topic.
Hi Eric, On Mon, 15 Oct 2018, Eric Sunshine wrote: > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > When a user is registered in a Windows domain, it is really easy to > > obtain the email address. So let's do that. > > [...] > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > diff --git a/compat/mingw.c b/compat/mingw.c > > @@ -1826,6 +1826,11 @@ static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) > > +char *mingw_query_user_email(void) > > +{ > > + return get_extended_user_info(NameUserPrincipal); > > +} > > diff --git a/ident.c b/ident.c > > @@ -168,6 +168,9 @@ const char *ident_default_email(void) > > + } else if ((email = query_user_email()) && email[0]) { > > + strbuf_addstr(&git_default_email, email); > > + free((char *)email); > > If query_user_email(), which calls get_extended_user_info(), returns > NULL, then we do nothing (and don't worry about freeing the result). > However, if query_user_email() returns a zero-length allocated string, > then this conditional will leak that string (due to the email[0] > check). From patch 2/3, we see in get_extended_user_info(): > > +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) > +{ > + if (GetUserNameExW(type, wbuffer, &len)) { > + char *converted = xmalloc((len *= 3)); > + if (xwcstoutf(converted, wbuffer, len) >= 0) > + return converted; > > that it may possibly return a zero-length string (due to the ">=0"). > Do we care about this potential leak (or is GetUserNameExW() > guaranteed never to return such a string)? Quite honestly, I think that this is so rare an instance (if it *can* happen at all) that I simply don't care ;-) Ciao, Dscho
diff --git a/compat/mingw.c b/compat/mingw.c index 623ff5daf5..44264fe3fd 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1826,6 +1826,11 @@ static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) return NULL; } +char *mingw_query_user_email(void) +{ + return get_extended_user_info(NameUserPrincipal); +} + struct passwd *getpwuid(int uid) { static unsigned initialized; diff --git a/compat/mingw.h b/compat/mingw.h index 571019d0bd..f31dcff2be 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -424,6 +424,8 @@ static inline void convert_slashes(char *path) int mingw_offset_1st_component(const char *path); #define offset_1st_component mingw_offset_1st_component #define PATH_SEP ';' +extern char *mingw_query_user_email(void); +#define query_user_email mingw_query_user_email #if !defined(__MINGW64_VERSION_MAJOR) && (!defined(_MSC_VER) || _MSC_VER < 1800) #define PRIuMAX "I64u" #define PRId64 "I64d" diff --git a/git-compat-util.h b/git-compat-util.h index 5f2e90932f..71779cb0ae 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -382,6 +382,10 @@ static inline char *git_find_last_dir_sep(const char *path) #define find_last_dir_sep git_find_last_dir_sep #endif +#ifndef query_user_email +#define query_user_email() NULL +#endif + #if defined(__HP_cc) && (__HP_cc >= 61000) #define NORETURN __attribute__((noreturn)) #define NORETURN_PTR diff --git a/ident.c b/ident.c index 327abe557f..33bcf40644 100644 --- a/ident.c +++ b/ident.c @@ -168,6 +168,9 @@ const char *ident_default_email(void) strbuf_addstr(&git_default_email, email); committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; author_ident_explicitly_given |= IDENT_MAIL_GIVEN; + } else if ((email = query_user_email()) && email[0]) { + strbuf_addstr(&git_default_email, email); + free((char *)email); } else copy_email(xgetpwuid_self(&default_email_is_bogus), &git_default_email, &default_email_is_bogus);