GNOME Bugzilla – Bug 656341
gtlsconsoleinteraction.c uses getpass() which isn't available on win32
Last modified: 2011-08-28 13:50:54 UTC
gio/tests/gtlsconsoleinteraction.c (merged with the rest of the GTlsCertificateDB code, https://bugzilla.gnome.org/show_bug.cgi?id=636572) uses getpass, which isn't available on win32. getpass(3) mentions: Present in SUSv2, but marked LEGACY. Removed in POSIX.1-2001. This function is obsolete. Do not use it. Possible replacements might be: - gnulib's getpass implementation: http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=lib/getpass.c - using fgets() - getpass replacement from OpenSC: http://www.opensc-project.org/opensc/browser/OpenSC/src/tools/util.c#L305 make[6]: Entering directory `/home/kalev/tmp/gnome/glib/gio/tests' CCLD socket-client.exe gtlsconsoleinteraction.o: In function `g_tls_console_interaction_ask_password': /home/kalev/tmp/gnome/glib/gio/tests/gtlsconsoleinteraction.c:45: undefined reference to `_getpass'
Created attachment 193641 [details] [review] patch: use fgets() instead of getpass() on G_OS_WIN32
Review of attachment 193641 [details] [review]: Looks good to merge. Needs a descriptive commit message that follows the glib style.
Review of attachment 193641 [details] [review]: ::: gio/tests/gtlsconsoleinteraction.c @@ +42,3 @@ +getpass (const gchar *prompt) +{ + gchar *result = g_malloc (128); BTW, it's not really that important, but if you really want to mirror the getpass() behavior you'd use: static gchar result[128]; return fgets (result, 128, stdin); </pedantic>
Comment on attachment 193641 [details] [review] patch: use fgets() instead of getpass() on G_OS_WIN32 It should print the prompt string, like getpass does. It should also print something like: "Warning: what you type will be visible".
Created attachment 193868 [details] [review] Replace getpass() by using getch() A different patch which uses getch() to emulate getpass() on WIN32. Compared to gets(), using getch() is better because it doesn't echo the characters back to the terminal; the downside is that this makes the patch slightly more involved than Sam's.
What about just disabling the test on win32? That means moving socket-client in Makefile.am inside the if OS_UNIX define. That way you don't have to emulate getpass, which is a flawed choice anyway as per the comment in gtlsconsoleinteraction.c.
(In reply to comment #6) > What about just disabling the test on win32? Disabling tests on win32 is a great way to accidentally break APIs on win32, and we do too much of that as is. It's just a test program. It doesn't have to be super secure or super pretty. The getch patch is probably fine (though I haven't actually looked at it yet).
Comment on attachment 193868 [details] [review] Replace getpass() by using getch() >+ if (prompt) >+ g_printf ("%s", prompt); need to fflush() afterward >+ if (buf[i] == '\r') is this right? you check for '\r' here, but print "\n" below... do you have a gnome git account or will you need someone else to commit this?
Thanks for the review, Dan! Yes, I do have an account and could push if you think it looks fine. > >+ if (prompt) > >+ g_printf ("%s", prompt); > > need to fflush() afterward Fixed for both of the printfs. > >+ if (buf[i] == '\r') > > is this right? you check for '\r' here, but print "\n" below... I believe so. On win32, the end of line marker is \r\n, so I'm just comparing the first character in that sequence. As for the printf, if stdout is in text mode (which is the default for stdout, AFAIK), \n gets implicitly converted to the platform-specific end of line marker (\r\n). On win32, fopen() defaults to the text mode, and takes an additional 'b' for doing binary I/O. In binary mode, the C runtime doesn't mangle the end-of-line characters like it does in the default text mode.
Created attachment 194959 [details] [review] Updated the getch() patch according to Dan's comments Added the fflush() calls Dan pointed out and did another small change to make it behave the same way as glibc's getpass() does for NULL arguments.
Comment on attachment 194959 [details] [review] Updated the getch() patch according to Dan's comments >+ g_printf ("\n"); >+ fflush (stdout); you don't need this one; stdout gets flushed automatically when you print the newline. so, remove that and it's good to commit
Review of attachment 194959 [details] [review]: Thanks, pushed to master as 41e4db807.