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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: May 19, 2011, 12:17 UTC
Message-ID: <BANLkTinPHeSfZXRb7pqt7-XWkR5fH=wAjg@mail.gmail.com>
In-Reply-To: <563395AE-A3E5-45FF-9063-F807C2CE3AD0@gieschke.de>

On Thu, May 19, 2011 at 1:37 PM, Rafael Gieschke <rafael@gieschke.de> wrote:

>
> If NO_GETPASS is set, getpass is provided in compat/getpass.c from
> https://github.com/CyanogenMod/android_external_dropbear/raw/master/netbsd_getpass.c
> (getpass was renamed to gitgetpass).

Nice.

But I can't help to think that this implementation of getpass looks a bit heavy, especially since we already have our own getpass implementation in compat/mingw.c.

Do we really need two implementations? Wouldn't it be better to factor out the mingw-version to a separate source file, and then improve it?

Windows doesn't have /dev/tty, but the logic in this version handles that by using stdin/stderr instead. The signal-stuff has a comment that indicates it might not even be correct. tcgetattr/tcsetattr isn't supported on Windows, but it's not needed if we use getch (as the version in compat/mingw.c does). POSIX/curses getch respects the echo-setting, while Windows getch never echo.

Given the information above, it sounds to me like we can enhance the version we already have to behave as it should also on non-Windows platforms, without having to maintain two versions. But it might not be worth it, given the simplicity of the Windows version and the increased dependency of curses, I dunno.

As a Windows-guy, I'm not entirely comfortable with putting something in compat/[function-name].c that is only portable to POSIX-platforms. This is not the current trend, but that might not be a conscious choice.

Show 46 quoted lines

> diff --git a/compat/getpass.c b/compat/getpass.c
> new file mode 100644
> index 0000000..e13f29f
> --- /dev/null
> +++ b/compat/getpass.c
> @@ -0,0 +1,114 @@
> +/*     $NetBSD: getpass.c,v 1.15 2003/08/07 16:42:50 agc Exp $ */
> +
> +/*
> + * Copyright (c) 1988, 1993
> + *     The Regents of the University of California.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the University nor the names of its contributors
> + *    may be used to endorse or promote products derived from this software
> + *    without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#if 0
> +#include <sys/cdefs.h>
> +#if defined(LIBC_SCCS) && !defined(lint)
> +#if 0
> +static char sccsid[] = "@(#)getpass.c  8.1 (Berkeley) 6/4/93";
> +#else
> +__RCSID("$NetBSD: getpass.c,v 1.15 2003/08/07 16:42:50 agc Exp $");
> +#endif
> +#endif /* LIBC_SCCS and not lint */

We usually don't keep dead code around. Since you've already edited the file, perhaps you should just delete those lines?

Show 6 quoted lines

> +
> +#if 0
> +#ifdef __weak_alias
> +__weak_alias(getpass,_getpass)
> +#endif
> +#endif

Same here.

Show 11 quoted lines

> +
> +char *
> +gitgetpass(prompt)
> +       const char *prompt;
> +{
> +       struct termios term;
> +       int ch;
> +       char *p;
> +       FILE *fp, *outfp;
> +       int echo;
> +       static char buf[_PASSWORD_LEN + 1];

Is _PASSWORD_LEN portable? A google search of "site:opengroup.org _PASSWORD_LEN " returns nothing...

The getpass implementation in compat/mingw.c uses a strbuf, so it doesn't have a limit on the password-length. Perhaps that would be an improvement here?

Show 5 quoted lines

> +       sigset_t oset, nset;
> +
> +#if 0
> +       _DIAGASSERT(prompt != NULL);
> +#endif

Again, dead code.

Show 6 quoted lines

> +
> +       /*
> +        * read and write to /dev/tty if possible; else read from
> +        * stdin and write to stderr.
> +        */
> +       if ((outfp = fp = fopen(_PATH_TTY, "w+")) == NULL) {

Is _PATH_TTY portable? A google search of "site:opengroup.org _PATH_TTY" returns nothing...