# [PATCH 0/3] Provide a useful default user ident on Windows 12 messages from 2018-10-15 to 2018-10-16. Participants: Johannes Schindelin via GitGitGadget, Eric Sunshine, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/49567 ## Johannes Schindelin via GitGitGadget, 2018-10-15 09:47 Subject: [PATCH 0/3] Provide a useful default user ident on Windows Message-ID: URL: https://gitlist.dev/e/pull.45.git.gitgitgadget%40gmail.com ``` On Linux, we use the gecos information to come up with a sensible user name/email. On Windows, there is no gecos. But there is something comparable, and with these patches, we use it. This has been carried in Git for Windows for three years, and is considered mature. Johannes Schindelin (3): getpwuid(mingw): initialize the structure only once getpwuid(mingw): provide a better default for the user name mingw: use domain information for default email compat/mingw.c | 60 +++++++++++++++++++++++++++++++++++++++++------ compat/mingw.h | 2 ++ git-compat-util.h | 4 ++++ ident.c | 3 +++ 4 files changed, 62 insertions(+), 7 deletions(-) base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-45%2Fdscho%2Fdefault-ident-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-45/dscho/default-ident-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/45 -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2018-10-15 09:47 Subject: [PATCH 1/3] getpwuid(mingw): initialize the structure only once Message-ID: <63373dd91d0be324273ab34ff49d630453954173.1539596822.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/63373dd91d0be324273ab34ff49d630453954173.1539596822.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Signed-off-by: Johannes Schindelin --- compat/mingw.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 18caf21969..597781b370 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1800,16 +1800,27 @@ int mingw_getpagesize(void) struct passwd *getpwuid(int uid) { + static unsigned initialized; static char user_name[100]; - static struct passwd p; + static struct passwd *p; + DWORD len; - DWORD len = sizeof(user_name); - if (!GetUserName(user_name, &len)) + if (initialized) + return p; + + len = sizeof(user_name); + if (!GetUserName(user_name, &len)) { + initialized = 1; return NULL; - p.pw_name = user_name; - p.pw_gecos = "unknown"; - p.pw_dir = NULL; - return &p; + } + + p = xmalloc(sizeof(*p)); + p->pw_name = user_name; + p->pw_gecos = "unknown"; + p->pw_dir = NULL; + + initialized = 1; + return p; } static HANDLE timer_event; -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2018-10-15 09:47 Subject: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name Message-ID: <11025b1639785577924b2020fb20d076308e9c69.1539596822.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/11025b1639785577924b2020fb20d076308e9c69.1539596822.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin We do have the excellent GetUserInfoEx() function to obtain more detailed information of the current user (if the user is part of a Windows domain); Let's use it. Suggested by Lutz Roeder. To avoid the cost of loading Secur32.dll (even lazily, loading DLLs takes a non-neglibile amount of time), we use the established technique to load DLLs only when, and if, needed. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index 597781b370..623ff5daf5 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -5,6 +5,7 @@ #include "../strbuf.h" #include "../run-command.h" #include "../cache.h" +#include "win32/lazyload.h" #define HCAST(type, handle) ((type)(intptr_t)handle) @@ -1798,6 +1799,33 @@ int mingw_getpagesize(void) return si.dwAllocationGranularity; } +/* See https://msdn.microsoft.com/en-us/library/windows/desktop/ms724435.aspx */ +enum EXTENDED_NAME_FORMAT { + NameDisplay = 3, + NameUserPrincipal = 8 +}; + +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) +{ + DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW, + enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG); + static wchar_t wbuffer[1024]; + DWORD len; + + if (!INIT_PROC_ADDR(GetUserNameExW)) + return NULL; + + len = ARRAY_SIZE(wbuffer); + if (GetUserNameExW(type, wbuffer, &len)) { + char *converted = xmalloc((len *= 3)); + if (xwcstoutf(converted, wbuffer, len) >= 0) + return converted; + free(converted); + } + + return NULL; +} + struct passwd *getpwuid(int uid) { static unsigned initialized; @@ -1816,7 +1844,9 @@ struct passwd *getpwuid(int uid) p = xmalloc(sizeof(*p)); p->pw_name = user_name; - p->pw_gecos = "unknown"; + p->pw_gecos = get_extended_user_info(NameDisplay); + if (!p->pw_gecos) + p->pw_gecos = "unknown"; p->pw_dir = NULL; initialized = 1; -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2018-10-15 09:47 Subject: [PATCH 3/3] mingw: use domain information for default email Message-ID: <271a5f57cef00d24ee33e99839cf3890e561efe7.1539596822.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/271a5f57cef00d24ee33e99839cf3890e561efe7.1539596822.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin When a user is registered in a Windows domain, it is really easy to obtain the email address. So let's do that. Suggested by Lutz Roeder. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 5 +++++ compat/mingw.h | 2 ++ git-compat-util.h | 4 ++++ ident.c | 3 +++ 4 files changed, 14 insertions(+) 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); -- gitgitgadget ``` ## Eric Sunshine, 2018-10-15 14:25 Subject: Re: [PATCH 1/3] getpwuid(mingw): initialize the structure only once Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS3Bz3uaxWv23bQEXB7Ok%3DQ%2BV07mGc-Fysg-WU3DKKagA%40mail.gmail.com In-Reply-To: <63373dd91d0be324273ab34ff49d630453954173.1539596822.git.gitgitgadget@gmail.com> ``` On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget wrote: > Signed-off-by: Johannes Schindelin > --- > diff --git a/compat/mingw.c b/compat/mingw.c > @@ -1800,16 +1800,27 @@ int mingw_getpagesize(void) > struct passwd *getpwuid(int uid) > { > + static unsigned initialized; > static char user_name[100]; > - static struct passwd p; > + static struct passwd *p; > > + if (initialized) > + return p; > + > + len = sizeof(user_name); > + if (!GetUserName(user_name, &len)) { > + initialized = 1; > return NULL; > + } If GetUserName() fails, that failure is recorded (as "initialized=1" and 'p' is still NULL), so subsequent invocations just return NULL without doing any more work. Makes sense. > + p = xmalloc(sizeof(*p)); > + p->pw_name = user_name; > + p->pw_gecos = "unknown"; > + p->pw_dir = NULL; > + > + initialized = 1; > + return p; > } ``` ## Eric Sunshine, 2018-10-15 14:34 Subject: Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRbOOrC5_jCnR2ohMZWoYXfNqvWtj3m_JTpudJhV1mQng%40mail.gmail.com In-Reply-To: <11025b1639785577924b2020fb20d076308e9c69.1539596822.git.gitgitgadget@gmail.com> ``` On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget wrote: > We do have the excellent GetUserInfoEx() function to obtain more > detailed information of the current user (if the user is part of a > Windows domain); Let's use it. > [...] > Signed-off-by: Johannes Schindelin > --- > diff --git a/compat/mingw.c b/compat/mingw.c > @@ -1798,6 +1799,33 @@ int mingw_getpagesize(void) > +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) > +{ > + DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW, > + enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG); > + static wchar_t wbuffer[1024]; Does this need to be static? It's not being returned to the caller. > + len = ARRAY_SIZE(wbuffer); > + if (GetUserNameExW(type, wbuffer, &len)) { > + char *converted = xmalloc((len *= 3)); > + if (xwcstoutf(converted, wbuffer, len) >= 0) > + return converted; > + free(converted); > + } If xwcstoutf() fails, 'converted' is freed; otherwise, the allocated 'converted' is stored in the caller's statically held 'passwd' struct. Okay. > + return NULL; > +} ``` ## Eric Sunshine, 2018-10-15 14:41 Subject: Re: [PATCH 3/3] mingw: use domain information for default email Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRaBRP9f3sR8suZxC%2BY2_UZaBMbMcpjsRUY%2BmGN%2BnyXkQ%40mail.gmail.com In-Reply-To: <271a5f57cef00d24ee33e99839cf3890e561efe7.1539596822.git.gitgitgadget@gmail.com> ``` On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget 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 > --- > 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)? ``` ## Junio C Hamano, 2018-10-16 03:59 Subject: Re: [PATCH 3/3] mingw: use domain information for default email Message-ID: URL: https://gitlist.dev/e/xmqqbm7u35kb.fsf%40gitster-ct.c.googlers.com In-Reply-To: <271a5f57cef00d24ee33e99839cf3890e561efe7.1539596822.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" 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. ``` ## Johannes Schindelin, 2018-10-16 12:34 Subject: Re: [PATCH 3/3] mingw: use domain information for default email Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1810161433220.4546%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Eric, On Mon, 15 Oct 2018, Eric Sunshine wrote: > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget > 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 > > --- > > 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 ``` ## Johannes Schindelin, 2018-10-16 12:38 Subject: Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1810161436480.4546%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Eric, On Mon, 15 Oct 2018, Eric Sunshine wrote: > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget > wrote: > > We do have the excellent GetUserInfoEx() function to obtain more > > detailed information of the current user (if the user is part of a > > Windows domain); Let's use it. > > [...] > > Signed-off-by: Johannes Schindelin > > --- > > diff --git a/compat/mingw.c b/compat/mingw.c > > @@ -1798,6 +1799,33 @@ int mingw_getpagesize(void) > > +static char *get_extended_user_info(enum EXTENDED_NAME_FORMAT type) > > +{ > > + DECLARE_PROC_ADDR(secur32.dll, BOOL, GetUserNameExW, > > + enum EXTENDED_NAME_FORMAT, LPCWSTR, PULONG); > > + static wchar_t wbuffer[1024]; > > Does this need to be static? It's not being returned to the caller. It does not. Fixed. > > + len = ARRAY_SIZE(wbuffer); > > + if (GetUserNameExW(type, wbuffer, &len)) { > > + char *converted = xmalloc((len *= 3)); Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call needs to receive `len + 1` to accommodate for the trailing NUL. Will fix, too. Ciao, Dscho > > + if (xwcstoutf(converted, wbuffer, len) >= 0) > > + return converted; > > + free(converted); > > + } > > If xwcstoutf() fails, 'converted' is freed; otherwise, the allocated > 'converted' is stored in the caller's statically held 'passwd' struct. > Okay. > > > + return NULL; > > +} > ``` ## Eric Sunshine, 2018-10-16 12:41 Subject: Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTvKicLKZO38AcVzXJJhZ1CajOhiADotRJ0V7EJXBicUA%40mail.gmail.com In-Reply-To: ``` On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin wrote: > On Mon, 15 Oct 2018, Eric Sunshine wrote: > > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget > > wrote: > > > + len = ARRAY_SIZE(wbuffer); > > > + if (GetUserNameExW(type, wbuffer, &len)) { > > > + char *converted = xmalloc((len *= 3)); > > Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call > needs to receive `len + 1` to accommodate for the trailing NUL. Will fix, > too. Or, xmallocz() (note the "z") which gives you overflow-checking of the +1 for free. ``` ## Johannes Schindelin, 2018-10-16 13:06 Subject: Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1810161506230.4546%40tvgsbejvaqbjf.bet In-Reply-To: ``` Hi Eric, On Tue, 16 Oct 2018, Eric Sunshine wrote: > On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin > wrote: > > On Mon, 15 Oct 2018, Eric Sunshine wrote: > > > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget > > > wrote: > > > > + len = ARRAY_SIZE(wbuffer); > > > > + if (GetUserNameExW(type, wbuffer, &len)) { > > > > + char *converted = xmalloc((len *= 3)); > > > > Oh wow. I *just* realized this. It is an off-by-one: the `xmalloc()` call > > needs to receive `len + 1` to accommodate for the trailing NUL. Will fix, > > too. > > Or, xmallocz() (note the "z") which gives you overflow-checking of the > +1 for free. Very good point! Thanks for the suggestion, Dscho ```