[3/3] mingw: use domain information for default email
diff mbox series

Message ID 271a5f57cef00d24ee33e99839cf3890e561efe7.1539596822.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Provide a useful default user ident on Windows
Related show

Commit Message

starlord via GitGitGadget Oct. 15, 2018, 9:47 a.m. UTC
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(+)

Comments

Eric Sunshine Oct. 15, 2018, 2:41 p.m. UTC | #1
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)?
Junio C Hamano Oct. 16, 2018, 3:59 a.m. UTC | #2
"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.
Johannes Schindelin Oct. 16, 2018, 12:34 p.m. UTC | #3
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

Patch
diff mbox series

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);