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

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: gitgitgadget@gmail.com, Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 3/3] mingw: use domain information for default email
Date: Tue, 16 Oct 2018 14:34:04 +0200 (DST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1810161433220.4546@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <CAPig+cRaBRP9f3sR8suZxC+Y2_UZaBMbMcpjsRUY+mGN+nyXkQ@mail.gmail.com>

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