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

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

[Ben Pfaff]

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, which
don't appear in the Gnulib POSIX implementation or the glibc
implementation.
---
I haven't pushed this yet; I'm posting it for review.

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

        Remove null checks for parameter declared as nonnull.
        * lib/getpass.c (getpass) [_WIN32]: Don't check whether 'prompt'
        argument is nonnull.

diff --git a/lib/getpass.c b/lib/getpass.c
index 3b0552ec58..f256bacdd8 100644
--- a/lib/getpass.c
+++ b/lib/getpass.c
@@ -194,11 +194,8 @@ getpass (const char *prompt)
   size_t i = 0;
   int c;
 
-  if (prompt)
-    {
-      fputs (prompt, stderr);
-      fflush (stderr);
-    }
+  fputs (prompt, stderr);
+  fflush (stderr);
 
   for (;;)
     {
@@ -220,11 +217,8 @@ getpass (const char *prompt)
         }
     }
 
-  if (prompt)
-    {
-      fputs ("\r\n", stderr);
-      fflush (stderr);
-    }
+  fputs ("\r\n", stderr);
+  fflush (stderr);
 
   return strdup (getpassbuf);
 }
-- 
2.28.0

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

[Bruno Haible]

Hi Ben,

> 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.

Bruno

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

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

[Bruno Haible]

Hi Ben,

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

Yes. This bit of added robustness doesn't cost much (neither in terms
of source code and maintenance, nor in terms of generated code).

> 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.

Looks good. Applied.

Bruno

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

[Ben Pfaff]

On Sun, Sep 13, 2020 at 09:05:32PM +0200, Bruno Haible wrote:
> > 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.
> 
> Looks good. Applied.

Thank you!