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

Commit Message

Johannes Schindelin via GitGitGadget Oct. 15, 2018, 9:47 a.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 compat/mingw.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Comments

Eric Sunshine Oct. 15, 2018, 2:34 p.m. UTC | #1

On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> ---
> 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;
> +}

Johannes Schindelin Oct. 16, 2018, 12:38 p.m. UTC | #2

Hi Eric,

On Mon, 15 Oct 2018, Eric Sunshine wrote:

> On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> <[e-mail]> 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 <[e-mail]>
> > ---
> > 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 Oct. 16, 2018, 12:41 p.m. UTC | #3

On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin
<[e-mail]> wrote:
> On Mon, 15 Oct 2018, Eric Sunshine wrote:
> > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> > <[e-mail]> 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, 1:06 p.m. UTC | #4

Hi Eric,

On Tue, 16 Oct 2018, Eric Sunshine wrote:

> On Tue, Oct 16, 2018 at 8:38 AM Johannes Schindelin
> <[e-mail]> wrote:
> > On Mon, 15 Oct 2018, Eric Sunshine wrote:
> > > On Mon, Oct 15, 2018 at 5:47 AM Johannes Schindelin via GitGitGadget
> > > <[e-mail]> 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

Patch

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;