Re: [PATCH] compat: add a getpass() compatibility function

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: May 20, 2011, 10:48 UTC
Message-ID: <BANLkTikRCXLJfOJFZ7j0TV_rxsySgQwSsw@mail.gmail.com>
In-Reply-To: <A4C82C4A-4A6A-412C-89D5-803F6DC85FD3@gieschke.de>

On Fri, May 20, 2011 at 12:06 PM, Rafael Gieschke <rafael@gieschke.de> wrote:

Show 22 quoted lines

>
> Am 19.05.2011 um 23:16 schrieb Erik Faye-Lund:
>>>
>>> Well, those platforms would currently fail, since that's the limit on
>>> the string returned from getpass. Since that hasn't happened yet, I
>>> suspect that this is a very theoretical problem.
>>>
>>> If we created our own define we could fix such problems by setting
>>> NO_GETPASS and having a reasonable lengthy GIT_MAX_PASS. But let's
>>> leave that theoretical fix for when/if it turns out to be real, huh?
>>>
>>
>> Whoa: http://www.opengroup.org/csq/view.mhtml?norationale=1&noreferences=1&RID=sun%2FSE2%2F10
>>
>> It seems that Solaris has a MAX_PASS of 8... That should mean that
>> prompted passwords can't be above 8 characters there (without using
>> GIT_ASKPASS). Can this really be the case?
>
> Good find. At least under "SunOS opensolaris 5.11 snv_111b i86pc i386 i86pc" with packages from http://sunfreeware.com/indexintel10.html, I can confirm exactly this behavior. If you try to connect to an account with password > 8 chars, git-imap-send prints "IMAP command 'LOGIN <user> <pass>' returned response (NO) - incorrect password or account name", using <= 8 chars it works fine (couldn't check git clone on https because I only have an internal IP without NAT/proxy).
>
> So maybe it would make sense to define NO_GETPASS and use compat/getpass.c on Solaris in the Makefile?
>

Actually, I'm thinking that we should avoid ever using getpass: it (and MAX_PASS) seems to have been deprecated from POSIX long ago, and is even removed from the latest version of the specification. Instead, we should make our own function (based on your most recent patch?) that fills a caller-specified strbuf instead (git_getpass already have a static strbuf that we can fill).

By the way, I gave merging the POSIX and Win32 code-paths a stab, by implementing minimal tc[gs]etattr : http://repo.or.cz/w/git/kusma.git/shortlog/refs/heads/work/win32-termios

It's close to working, but the fact that we set stdin to binary mode by default makes it misbehave and append '\r' to the end of the input. I have some patches to try to avoid the nasty _setmode(..., _O_BINARY)-hack in our mingw main-wrapper; perhaps this is the right timing to finish those?

Another thing I tried, was to make the call to fopen use the path "con" (which is the path to the terminal on Windows) instead of "/dev/tty"; this made SetConsoleMode fail, claiming that the handle was invalid. This failure baffles me, because the handle was valid a few lines earlier, when I called GetConsoleMode... The idea was that the handle would be explicitly opened in text-mode (since there's no 'b'-character in the mode-string), but I forgot that we also set _fmode to _O_BINARY. Ugghhh, Git's handling of new-lines on Windows is hacky... :P

So, it seems to me that the _setmode-hack should be killed to get this code POSIX/Windows-unified (or the _fmode-hack and the SetConsoleMode-failure should be combated).