Re: [PATCH] getpass: Do not check for nonnull prompt argument in Win32 implementation.

[Ben Pfaff]

On Sun, Sep 13, 2020 at 10:32:27AM +0200, Bruno Haible wrote:
> > The prompt parameter to getpass() is declared as nonnull (using a GCC
> > nonnull attribute), but the implementation checks whether it is null in
> > two places.  GCC warns about this.  This commit removes the checks
> 
> GCC warnings ought to help us make the code more robust. Removing the
> NULL check makes it less robust.
> 
> The problem has already occurred a couple of times:
> https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00050.html
> https://lists.gnu.org/archive/html/bug-gnulib/2018-08/msg00116.html
> https://lists.gnu.org/archive/html/bug-gnulib/2013-02/msg00060.html
> https://lists.gnu.org/archive/html/bug-gnulib/2009-12/msg00173.html
> 
> I would prefer that the same idiom gets used, that gets rid of the
> warning without removing the NULL check at run time.

Thanks.

It is a little complicated because there are three implementations of
getpass(), and two of them do not check the prompt:

  * The glibc implementation passes the prompt as %s to fprintf().  I
    guess that glibc will generally print "(null)", although I've seen
    GCC "optimize" similar things to fputs() in the past, which will
    dereference null.  I guess I won't worry about it.

  * The POSIX implementation in gnulib passes the prompt to fputs()
    without checking for null.  I guess we should fix it.

I like the trick from one of your references about defining
_GL_ARG_NONNULL to empty.  That works OK here.

So, how about like this?

-8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp@cs.stanford.edu>
Date: Sat, 12 Sep 2020 15:54:36 -0700
Subject: [PATCH] getpass: Check for nonnull prompt argument while avoiding
 warnings.

The prompt parameter to getpass() is declared as nonnull (using a GCC
nonnull attribute).  Gnulib contains two implementations of this function,
one for POSIX, one for Windows.  The Windows implementation checked for
a nonnull prompt, which caused a GCC warning.  This commit fixes that by
avoiding the nonnull attribute when building getpass.c.  The POSIX
implementation did not check for a nonnull prompt.  This commit increases
the robustness by adding such a check.

2020-09-12  Ben Pfaff  <blp@cs.stanford.edu>

        Check for nonnull prompt argument while avoiding warnings.
        * lib/getpass.c (_GL_ARG_NONNULL): Define to empty.
        (getpass) [!_WIN32]: Print prompt only if nonnull.

        Use __builtin_signbit* with clang.
diff --git a/lib/getpass.c b/lib/getpass.c
index 3b0552ec58..ca528fdc09 100644
--- a/lib/getpass.c
+++ b/lib/getpass.c
@@ -16,6 +16,9 @@
    with this program; if not, see <https://www.gnu.org/licenses/>.  */
 
 #ifndef _LIBC
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   warns for the null checks on 'prompt' below.  */
+# define _GL_ARG_NONNULL(params)
 # include <config.h>
 #endif
 
@@ -124,9 +127,12 @@ getpass (const char *prompt)
     }
 # endif
 
-  /* Write the prompt.  */
-  fputs_unlocked (prompt, out);
-  fflush_unlocked (out);
+  if (prompt)
+    {
+      /* Write the prompt.  */
+      fputs_unlocked (prompt, out);
+      fflush_unlocked (out);
+    }
 
   /* Read the password.  */
   nread = getline (&buf, &bufsize, in);
-- 
2.28.0