threads / patch / 49567

patch, 3 partsProvide a useful default user ident on Windows

Subject: [PATCH 0/3] Provide a useful default user ident on Windows

## tl;dr

12 messages between Oct 15, 2018 and Oct 16, 2018. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Johannes Schindelin via GitGitGadget· Oct 15, 2018, 09:47 UTC · lore

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· Oct 15, 2018, 09:47 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 1/3] getpwuid(mingw): initialize the structure only once

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
Show changes to compat/mingw.c +18 −7
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
Eric Sunshine· Oct 15, 2018, 14:25 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 1/3] getpwuid(mingw): initialize the structure only once

On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 19 quoted lines
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 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.

Show 8 quoted lines
> +       p = xmalloc(sizeof(*p));
> +       p->pw_name = user_name;
> +       p->pw_gecos = "unknown";
> +       p->pw_dir = NULL;
> +
> +       initialized = 1;
> +       return p;
>  }
Johannes Schindelin via GitGitGadget· Oct 15, 2018, 09:47 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 2/3] getpwuid(mingw): provide a better default for the user name

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)
Show changes to compat/mingw.c +31 −1
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
Eric Sunshine· Oct 15, 2018, 14:34 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name

On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 13 quoted lines
> 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 <johannes.schindelin@gmx.de>
> ---
> 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.
Show 7 quoted lines
> +       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;
> +}
Johannes Schindelin· Oct 16, 2018, 12:38 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name

Hi Eric,
On Mon, 15 Oct 2018, Eric Sunshine wrote:
Show 17 quoted lines
> On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> 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 <johannes.schindelin@gmx.de>
> > ---
> > 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

Show 12 quoted lines
> > +               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· Oct 16, 2018, 12:41 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name

On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 10 quoted lines
> 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:
> > > +       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· Oct 16, 2018, 13:06 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/3] getpwuid(mingw): provide a better default for the user name

Hi Eric,
On Tue, 16 Oct 2018, Eric Sunshine wrote:
Show 15 quoted lines
> On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > 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:
> > > > +       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

Johannes Schindelin via GitGitGadget· Oct 15, 2018, 09:47 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH 3/3] mingw: use domain information for default email

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 compat/mingw.c    | 5 +++++
 compat/mingw.h    | 2 ++
 git-compat-util.h | 4 ++++
 ident.c           | 3 +++
 4 files changed, 14 insertions(+)
Show changes to 4 files +14 −0

compat/mingw.c, compat/mingw.h, git-compat-util.h, ident.c

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· Oct 15, 2018, 14:41 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 3/3] mingw: use domain information for default email

On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 16 quoted lines
> 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· Oct 16, 2018, 12:34 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/3] mingw: use domain information for default email

Hi Eric,
On Mon, 15 Oct 2018, Eric Sunshine wrote:
Show 35 quoted lines
> 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

Junio C Hamano· Oct 16, 2018, 03:59 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH 3/3] mingw: use domain information for default email

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines
> +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.

← back to recent threads