Re: [PATCH] compat: add a getpass() compatibility function
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- May 19, 2011, 18:07 UTC
- Message-ID
- <BANLkTimDW8W13Wm8i+n0ww9jCeHsXc__iA@mail.gmail.com>
- In-Reply-To
- <7v62p68ut0.fsf@alter.siamese.dyndns.org>
On Thu, May 19, 2011 at 7:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Rafael Gieschke <rafael@gieschke.de> writes: > >>> 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. > > Probably a properly abstracted common version would look like a function > that calls four platform-dependent helper funcions in this order: > > 0. prompt > 1. start "noecho" mode > 2. get whole line > 3. exit "noecho" mode > > where Windows may use stderr for 0, have noop() implementation for 1 and > 3, use _getch() that does not echo for 2, while POSIX may write to > /dev/tty for 0, use tc[gs]etattr() with perhaps some signal settings > sprinkled in for 1 and 3. > > So I don't see a need for Windows to emulate tc[g]setattr nor curses in > order to get a generic getpass() abstraction between two platforms. >
When I think about it a bit more, it feels a bit pointless: 0. is identical (fputs) 1. is different (tc[gs]etattr vs nop) 2. is different (getc vs _getch) 3. is different (tcsetattr vs nop)
So there's probably not much code to share here. There's a bit of logic, but I'm not entirely sure this should be the same either, because on Windows we have to take care of '\r' (since we open stdin in binary mode at start-up).
But looking at the implementations, there's one thing that strike me: both the netbsd and uclibc implementations fill a static buffer, while our windows-version returns an allocated buffer. Reading POSIX, it's not entirely clear if the returned pointer should be free'd or not: http://pubs.opengroup.org/onlinepubs/007908799/xsh/getpass.html
But there is a hint: the limit of PASS_MAX bytes indicate that the buffer is expected to be statically allocated. So perhaps we should do this to prevent a leak?
diff --git a/compat/mingw.c b/compat/mingw.c index 878b1de..dba3e64 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1668,17 +1668,19 @@ int link(const char *oldpath, const char *newpath) char *getpass(const char *prompt) { - struct strbuf buf = STRBUF_INIT; + static char buf[PASS_MAX]; + int i; fputs(prompt, stderr); - for (;;) { + for (i = 0; i < PASS_MAX - 1; ++i) { char c = _getch(); if (c == '\r' || c == '\n') break; - strbuf_addch(&buf, c); + buf[i] = c; } + buf[i] = '\0'; fputs("\n", stderr); - return strbuf_detach(&buf, NULL); + return buf; } pid_t waitpid(pid_t pid, int *status, unsigned options) diff --git a/compat/mingw.h b/compat/mingw.h index 62eccd3..e37d557 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -48,6 +48,8 @@ typedef int socklen_t; #define EAFNOSUPPORT WSAEAFNOSUPPORT #define ECONNABORTED WSAECONNABORTED +#define PASS_MAX 512 + struct passwd { char *pw_name; char *pw_gecos;