Re: [Bug-gnulib] proposed patch for getpass and unlocked-io

[Paul Eggert]

Bruno Haible <address@hidden> writes:

> I don't think supporting wide-character I/O in gnulib is worth the
> complexities.

Good point.  I'll rip it out.

> > +         if (!_LIBC)
> 
> Why this? The customary idiom throughout glibc is to use
> 
>     #  if !_LIBC

OK, I'll switch to the usual glibc idiom here.  Just FYI, my own
preference is to avoid #if, all other things being equal.  An
advantage of avoiding #if is that you have better syntax- and
type-checking on code even if it won't be run on your platform.
This has saved me time in the past.

> >     [!_LIBC]: Unconditionally do the fseek, since we don't know what
> >     stream might go where.
> 
> I don't understand this. Larry Jones explained that the problem is inside
> the stdio, not inside the kernel or tty driver, therefore the test
> (out == in) is the right one.

My worry here was about POSIX, not the C standard.  The C standard
doesn't have fileno, so it need not worry about the case where out !=
in && fileno (out) == fileno (in), a case that could happen here.
POSIX does have fileno, and it places some constraints on how
applications must deal with file descriptors and stream pointers in
that case.  To be honest, I haven't thought through all the
possibilities: the POSIX rules are pretty complicated, and have
changed significantly in various POSIX releases.  Also, we want the
code to be portable to older implementations.  So I decided to be
conservative.  Perhaps there's a solid argument as to why the out ==
in optimization will work on all practical porting targets, but I
don't know what it is offhand.

I just noticed another problem: the fseek is done only if buf != NULL
&& 0 <= nread && buf[nread - 1] == '\n'.  It should also be done if
out == stderr, as otherwise if the program later outputs to stderr
then the behavior will be undefined.

I guess it's simplest just to do the fseek unconditionally -- on
non-glibc platforms, anyway.  I also considered removing the '#if
!_LIBC' test around the fseek, but rejected that since I figure the
code must be absent from glibc for a reason.

> How about a more descriptive argument name, like 'void *fp' or 'void *tty' ?

I was following the libc code here, and I'm trying to minimize changes
to it.

> The distribution of spaces here is inconsistent with the rest of the file.

Thanks for catching that.

I installed this:

Index: ChangeLog
===================================================================
RCS file: /cvsroot/gnulib/gnulib/ChangeLog,v
retrieving revision 1.105
diff -p -u -r1.105 ChangeLog
--- ChangeLog   6 Oct 2003 20:32:37 -0000       1.105
+++ ChangeLog   8 Oct 2003 20:33:13 -0000
@@ -1,3 +1,7 @@
+2003-10-08  Paul Eggert  <address@hidden>
+
+       * modules/getpass: Depend on stdbool.
+
 2003-10-06  Bruno Haible  <address@hidden>
 
        * modules/version-etc-2: Remove file.
Index: lib/ChangeLog
===================================================================
RCS file: /cvsroot/gnulib/gnulib/lib/ChangeLog,v
retrieving revision 1.581
diff -p -u -r1.581 ChangeLog
--- lib/ChangeLog       6 Oct 2003 20:33:40 -0000       1.581
+++ lib/ChangeLog       8 Oct 2003 20:33:19 -0000
@@ -1,3 +1,40 @@
+2003-10-08  Paul Eggert  <address@hidden>
+
+       Merge getpass from libc, plus a few fixes.
+
+       * getpass.c (HAVE_STDIO_EXT) [_LIBC]: Define to 1.
+       Include <stdbool.h>.
+       Include <stdio_ext.h> if HAVE_STDIO_H, otherwise define
+       __fsetlocking to empty.
+       [_LIBC]: Do not include "getline.h" or "unlocked-io.h", but
+       do include <bits/libc-lock.h>.
+       Do not include <fcntl.h>; not needed.
+       [_LIBC]: Include <wchar.h>.
+       (NOTCANCEL_MODE): New macro.
+       (flockfile, funlockfile) [_LIBC]: New macros.
+       (__libc_cleanup_push, __libc_cleanup_pop, __getline, __tcgetattr)
+       [!_LIBC]: New macros.
+       (call_fclose): New function.
+       (getpass): Use it.  Save tty stream separately; this simplifies the
+       code and makes it more reliable if stdin happens to equal stdout.
+       Invoke __fsetlocking on tty.
+       Handle thread cancellation if needed.
+       Namespace cleanup (use __tcgetattr, __getline).
+       Use bool for Booleans.
+       [USE_IN_LIBIO]: Handle wide streams.
+       [!_LIBC]: Unconditionally do the fseek, since we don't know what
+       stream might go where.
+
+       * unlocked-io.h: Include <stdio.h>, so that the caller
+       doesn't have to include <stdio.h> before us.
+       (clearerr_unlocked, feof_unlocked, ferror_unlocked,
+       fflush_unlocked, fgets_unlocked, fputc_unlocked, fputs_unlocked,
+       fread_unlocked, fwrite_unlocked, getc_unlocked, getchar_unlocked,
+       putc_unlocked, putchar_unlocked): Define to the unlocked counterpart
+       if not declared, so that we can use getpass.c code from libc without
+       rewriting it.
+       (flockfile, ftrylockfile, funlockfile): New macros.
+
 2003-10-06  Bruno Haible  <address@hidden>
 
        * version-etc-2.h: Remove file.
Index: lib/getpass.c
===================================================================
RCS file: /cvsroot/gnulib/gnulib/lib/getpass.c,v
retrieving revision 1.5
diff -p -u -r1.5 getpass.c
--- lib/getpass.c       1 Oct 2003 11:11:02 -0000       1.5
+++ lib/getpass.c       8 Oct 2003 20:33:19 -0000
@@ -19,12 +19,53 @@
 # include <config.h>
 #endif
 
+#if _LIBC
+# define HAVE_STDIO_EXT_H 1
+#endif
+
+#include <stdbool.h>
+
 #include <stdio.h>
+#if HAVE_STDIO_EXT_H
+# include <stdio_ext.h>
+#else
+# define __fsetlocking(stream, type) /* empty */
+#endif
+#if !_LIBC
+# include "getline.h"
+#endif
+
 #include <termios.h>
 #include <unistd.h>
-#include <fcntl.h>
-#include "getline.h"
-#include "unlocked-io.h"
+
+#if _LIBC
+# include <wchar.h>
+#endif
+
+#if _LIBC
+# define NOTCANCEL_MODE "c"
+#else
+# define NOTCANCEL_MODE
+#endif
+
+#if _LIBC
+# define flockfile(s) _IO_flockfile (s)
+# define funlockfile(s) _IO_funlockfile (s)
+#else
+# include "unlocked-io.h"
+#endif
+
+#if _LIBC
+# include <bits/libc-lock.h>
+#else
+# define __libc_cleanup_push(function, arg) /* empty */
+# define __libc_cleanup_pop(execute) /* empty */
+#endif
+
+#if !_LIBC
+# define __getline getline
+# define __tcgetattr tcgetattr
+#endif
 
 /* It is desirable to use this bit on systems that have it.
    The only bit of terminal state we want to twiddle is echoing, which is
@@ -35,12 +76,20 @@
 # define TCSASOFT 0
 #endif
 
+static void
+call_fclose (void *arg)
+{
+  if (arg != NULL)
+    fclose (arg);
+}
+
 char *
 getpass (const char *prompt)
 {
+  FILE *tty;
   FILE *in, *out;
   struct termios s, t;
-  int tty_changed;
+  bool tty_changed;
   static char *buf;
   static size_t bufsize;
   ssize_t nread;
@@ -48,18 +97,29 @@ getpass (const char *prompt)
   /* Try to write to and read from the terminal if we can.
      If we can't open the terminal, use stderr and stdin.  */
 
-  in = fopen ("/dev/tty", "w+");
-  if (in == NULL)
+  tty = fopen ("/dev/tty", "w+" NOTCANCEL_MODE);
+  if (tty == NULL)
     {
       in = stdin;
       out = stderr;
     }
   else
-    out = in;
+    {
+      /* We do the locking ourselves.  */
+      __fsetlocking (tty, FSETLOCKING_BYCALLER);
+
+      out = in = tty;
+    }
+
+  /* Make sure the stream we opened is closed even if the thread is
+     canceled.  */
+  __libc_cleanup_push (call_fclose, tty);
+
+  flockfile (out);
 
   /* Turn echoing off if it is on now.  */
 
-  if (tcgetattr (fileno (in), &t) == 0)
+  if (__tcgetattr (fileno (in), &t) == 0)
     {
       /* Save the old one. */
       s = t;
@@ -68,14 +128,35 @@ getpass (const char *prompt)
       tty_changed = (tcsetattr (fileno (in), TCSAFLUSH|TCSASOFT, &t) == 0);
     }
   else
-    tty_changed = 0;
+    tty_changed = false;
 
   /* Write the prompt.  */
-  fputs (prompt, out);
-  fflush (out);
+#ifdef USE_IN_LIBIO
+  if (_IO_fwide (out, 0) > 0)
+    __fwprintf (out, L"%s", prompt);
+  else
+#endif
+    fputs_unlocked (prompt, out);
+  fflush_unlocked (out);
 
   /* Read the password.  */
-  nread = getline (&buf, &bufsize, in);
+  nread = __getline (&buf, &bufsize, in);
+
+#if !_LIBC
+  /* As far as is known, glibc doesn't need this no-op fseek.  */
+
+  /* According to the C standard, input may not be followed by output
+     on the same stream without an intervening call to a file
+     positioning function.  Suppose in == out; then without this fseek
+     call, on Solaris, HP-UX, AIX, OSF/1, the previous input gets
+     echoed, whereas on IRIX, the following newline is not output as
+     it should be.  POSIX imposes similar restrictions if fileno (in)
+     == fileno (out).  The POSIX restrictions are tricky and change
+     from POSIX version to POSIX version, so play it safe and invoke
+     fseek even if in != out.  */
+  fseek (out, 0, SEEK_CUR);
+#endif
+
   if (buf != NULL)
     {
       if (nread < 0)
@@ -86,16 +167,13 @@ getpass (const char *prompt)
          buf[nread - 1] = '\0';
          if (tty_changed)
            {
-             /* Write the newline that was not echoed.
-                But before doing that, do a no-op fseek.  According to the C
-                standard, input may not be followed by output on the same
-                stream without an intervening call to a file positioning
-                function.  Without this fseek() call, on Solaris, HP-UX,
-                AIX, OSF/1, the previous input gets echoed, whereas on IRIX,
-                the following newline is not output as it should.  */
-             if (out == in)
-               fseek (out, 0, SEEK_CUR);
-             putc ('\n', out);
+             /* Write the newline that was not echoed.  */
+#ifdef USE_IN_LIBIO
+             if (_IO_fwide (out, 0) > 0)
+               putwc_unlocked (L'\n', out);
+             else
+#endif
+               putc_unlocked ('\n', out);
            }
        }
     }
@@ -104,9 +182,11 @@ getpass (const char *prompt)
   if (tty_changed)
     (void) tcsetattr (fileno (in), TCSAFLUSH|TCSASOFT, &s);
 
-  if (in != stdin)
-    /* We opened the terminal; now close it.  */
-    fclose (in);
+  funlockfile (out);
+
+  __libc_cleanup_pop (0);
+
+  call_fclose (tty);
 
   return buf;
 }
Index: lib/unlocked-io.h
===================================================================
RCS file: /cvsroot/gnulib/gnulib/lib/unlocked-io.h,v
retrieving revision 1.2
diff -p -u -r1.2 unlocked-io.h
--- lib/unlocked-io.h   14 Jul 2003 16:36:07 -0000      1.2
+++ lib/unlocked-io.h   8 Oct 2003 20:33:20 -0000
@@ -27,63 +27,106 @@
 
 # if USE_UNLOCKED_IO
 
-/* These are wrappers for functions/macros from GNU libc.
+/* These are wrappers for functions/macros from the GNU C library, and
+   from other C libraries supporting POSIX's optional thread-safe functions.
+
    The standard I/O functions are thread-safe.  These *_unlocked ones are
    more efficient but not thread-safe.  That they're not thread-safe is
-   fine since all of the applications in this package are single threaded.  */
+   fine since all of the applications in this package are single threaded.
+
+   Also, some code that is shared with the GNU C library may invoke
+   the *_unlocked functions directly.  On hosts that lack those
+   functions, invoke the non-thread-safe versions instead.  */
+
+#  include <stdio.h>
 
 #  if HAVE_DECL_CLEARERR_UNLOCKED
 #   undef clearerr
 #   define clearerr(x) clearerr_unlocked (x)
+#  else
+#   define clearerr_unlocked(x) clearerr (x)
 #  endif
 #  if HAVE_DECL_FEOF_UNLOCKED
 #   undef feof
 #   define feof(x) feof_unlocked (x)
+#  else
+#   define feof_unlocked(x) feof (x)
 #  endif
 #  if HAVE_DECL_FERROR_UNLOCKED
 #   undef ferror
 #   define ferror(x) ferror_unlocked (x)
+#  else
+#   define ferror_unlocked(x) ferror (x)
 #  endif
 #  if HAVE_DECL_FFLUSH_UNLOCKED
 #   undef fflush
 #   define fflush(x) fflush_unlocked (x)
+#  else
+#   define fflush_unlocked(x) fflush (x)
 #  endif
 #  if HAVE_DECL_FGETS_UNLOCKED
 #   undef fgets
 #   define fgets(x,y,z) fgets_unlocked (x,y,z)
+#  else
+#   define fgets_unlocked(x,y,z) fgets (x,y,z)
 #  endif
 #  if HAVE_DECL_FPUTC_UNLOCKED
 #   undef fputc
 #   define fputc(x,y) fputc_unlocked (x,y)
+#  else
+#   define fputc_unlocked(x,y) fputc (x,y)
 #  endif
 #  if HAVE_DECL_FPUTS_UNLOCKED
 #   undef fputs
 #   define fputs(x,y) fputs_unlocked (x,y)
+#  else
+#   define fputs_unlocked(x,y) fputs (x,y)
 #  endif
 #  if HAVE_DECL_FREAD_UNLOCKED
 #   undef fread
 #   define fread(w,x,y,z) fread_unlocked (w,x,y,z)
+#  else
+#   define fread_unlocked(w,x,y,z) fread (w,x,y,z)
 #  endif
 #  if HAVE_DECL_FWRITE_UNLOCKED
 #   undef fwrite
 #   define fwrite(w,x,y,z) fwrite_unlocked (w,x,y,z)
+#  else
+#   define fwrite_unlocked(w,x,y,z) fwrite (w,x,y,z)
 #  endif
 #  if HAVE_DECL_GETC_UNLOCKED
 #   undef getc
 #   define getc(x) getc_unlocked (x)
+#  else
+#   define getc_unlocked(x) getc (x)
 #  endif
 #  if HAVE_DECL_GETCHAR_UNLOCKED
 #   undef getchar
 #   define getchar() getchar_unlocked ()
+#  else
+#   define getchar_unlocked() getchar ()
 #  endif
 #  if HAVE_DECL_PUTC_UNLOCKED
 #   undef putc
 #   define putc(x,y) putc_unlocked (x,y)
+#  else
+#   define putc_unlocked(x,y) putc (x,y)
 #  endif
 #  if HAVE_DECL_PUTCHAR_UNLOCKED
 #   undef putchar
 #   define putchar(x) putchar_unlocked (x)
+#  else
+#   define putchar_unlocked(x) putchar (x)
 #  endif
+
+#  undef flockfile
+#  define flockfile(x) ((void) 0)
+
+#  undef ftrylockfile
+#  define ftrylockfile(x) 0
+
+#  undef funlockfile
+#  define funlockfile(x) ((void) 0)
 
 # endif /* USE_UNLOCKED_IO */
 #endif /* UNLOCKED_IO_H */
Index: m4/ChangeLog
===================================================================
RCS file: /cvsroot/gnulib/gnulib/m4/ChangeLog,v
retrieving revision 1.504
diff -p -u -r1.504 ChangeLog
--- m4/ChangeLog        6 Oct 2003 12:50:12 -0000       1.504
+++ m4/ChangeLog        8 Oct 2003 20:33:24 -0000
@@ -1,3 +1,7 @@
+2003-10-08  Paul Eggert  <address@hidden>
+
+       * getpass.m4 (gl_PREREQ_GETPASS): Check for stdio_ext.h.
+
 2003-10-06  Bruno Haible  <address@hidden>
 
        * fatal-signal.m4: New file.
Index: m4/getpass.m4
===================================================================
RCS file: /cvsroot/gnulib/gnulib/m4/getpass.m4,v
retrieving revision 1.3
diff -p -u -r1.3 getpass.m4
--- m4/getpass.m4       31 Jul 2003 14:51:31 -0000      1.3
+++ m4/getpass.m4       8 Oct 2003 20:33:24 -0000
@@ -1,4 +1,4 @@
-# getpass.m4 serial 2
+# getpass.m4 serial 3
 dnl Copyright (C) 2002-2003 Free Software Foundation, Inc.
 dnl This file is free software, distributed under the terms of the GNU
 dnl General Public License.  As a special exception to the GNU General
@@ -31,6 +31,7 @@ AC_DEFUN([gl_FUNC_GETPASS_GNU],
 
 # Prerequisites of lib/getpass.c.
 AC_DEFUN([gl_PREREQ_GETPASS], [
+  AC_CHECK_HEADERS_ONCE(stdio_ext.h)
   :
 ])
 
Index: modules/getpass
===================================================================
RCS file: /cvsroot/gnulib/gnulib/modules/getpass,v
retrieving revision 1.3
diff -p -u -r1.3 getpass
--- modules/getpass     20 Jan 2003 10:02:37 -0000      1.3
+++ modules/getpass     8 Oct 2003 20:33:25 -0000
@@ -8,6 +8,7 @@ m4/getpass.m4
 Depends-on:
 unlocked-io
 getline
+stdbool
 
 configure.ac:
 gl_FUNC_GETPASS