[v7,4/4] tests: replace fgets by xfgets

  With fortification enabled, fgets calls return result needs to be checked,
has it gets the __wur macro enabled.
---
Changes since v6:
 - Fixed support/Makefile ordering
 - Fixed header copyright date

 assert/test-assert-perr.c     |  8 +++++---
 assert/test-assert.c          |  8 +++++---
 stdio-common/test_rdwr.c      | 11 ++++-------
 support/Makefile              |  1 +
 support/xfgets.c              | 32 ++++++++++++++++++++++++++++++++
 support/xstdio.h              |  1 +
 sysdeps/pthread/tst-cancel6.c |  3 ++-
 7 files changed, 50 insertions(+), 14 deletions(-)
 create mode 100644 support/xfgets.c
  

On 2023-06-12 11:18, Frédéric Bérat wrote:
> With fortification enabled, fgets calls return result needs to be checked,
> has it gets the __wur macro enabled.
> ---
> Changes since v6:
>   - Fixed support/Makefile ordering
>   - Fixed header copyright date
> 
>   assert/test-assert-perr.c     |  8 +++++---
>   assert/test-assert.c          |  8 +++++---
>   stdio-common/test_rdwr.c      | 11 ++++-------
>   support/Makefile              |  1 +
>   support/xfgets.c              | 32 ++++++++++++++++++++++++++++++++
>   support/xstdio.h              |  1 +
>   sysdeps/pthread/tst-cancel6.c |  3 ++-
>   7 files changed, 50 insertions(+), 14 deletions(-)
>   create mode 100644 support/xfgets.c
> 
> diff --git a/assert/test-assert-perr.c b/assert/test-assert-perr.c
> index 8496db6ffd..09a4fcb6ef 100644
> --- a/assert/test-assert-perr.c
> +++ b/assert/test-assert-perr.c
> @@ -11,6 +11,8 @@
>   #include <string.h>
>   #include <setjmp.h>
>   
> +#include <support/xstdio.h>
> +
>   jmp_buf rec;
>   char buf[160];
>   
> @@ -70,15 +72,15 @@ main(void)
>       failed = 1; /* should not happen */
>   
>     rewind (stderr);
> -  fgets (buf, 160, stderr);
> +  xfgets (buf, 160, stderr);

Joseph, would you apply the same rationale for these tests too, i.e. 
since the test involves interaction with stdio and signals, would you 
avoid adding an xstdio abstraction here to test stdio directly?

Likewise for the stdio-common test here and in patch 3/4, what do you think?

Thanks,
Sid

>     if (!strstr(buf, strerror (1)))
>       failed = 1;
>   
> -  fgets (buf, 160, stderr);
> +  xfgets (buf, 160, stderr);
>     if (strstr (buf, strerror (0)))
>       failed = 1;
>   
> -  fgets (buf, 160, stderr);
> +  xfgets (buf, 160, stderr);
>     if (strstr (buf, strerror (2)))
>       failed = 1;
>   
> diff --git a/assert/test-assert.c b/assert/test-assert.c
> index 26b58d4dd3..25e264543b 100644
> --- a/assert/test-assert.c
> +++ b/assert/test-assert.c
> @@ -11,6 +11,8 @@
>   #include <string.h>
>   #include <setjmp.h>
>   
> +#include <support/xstdio.h>
> +
>   jmp_buf rec;
>   char buf[160];
>   
> @@ -72,15 +74,15 @@ main (void)
>       failed = 1; /* should not happen */
>   
>     rewind (stderr);
> -  fgets (buf, 160, stderr);
> +  xfgets (buf, 160, stderr);
>     if (!strstr (buf, "1 == 2"))
>       failed = 1;
>   
> -  fgets (buf, 160, stderr);
> +  xfgets (buf, 160, stderr);
>     if (strstr (buf, "1 == 1"))
>       failed = 1;
>   
> -  fgets (buf, 160, stderr);
> +  xfgets (buf, 160, stderr);
>     if (strstr (buf, "2 == 3"))
>       failed = 1;
>   
> diff --git a/stdio-common/test_rdwr.c b/stdio-common/test_rdwr.c
> index 7825ca9358..0544916eb1 100644
> --- a/stdio-common/test_rdwr.c
> +++ b/stdio-common/test_rdwr.c
> @@ -20,6 +20,7 @@
>   #include <stdlib.h>
>   #include <string.h>
>   
> +#include <support/xstdio.h>
>   
>   int
>   main (int argc, char **argv)
> @@ -49,7 +50,7 @@ main (int argc, char **argv)
>   
>     (void) fputs (hello, f);
>     rewind (f);
> -  (void) fgets (buf, sizeof (buf), f);
> +  xfgets (buf, sizeof (buf), f);
>     rewind (f);
>     (void) fputs (buf, f);
>     rewind (f);
> @@ -104,12 +105,8 @@ main (int argc, char **argv)
>     if (!lose)
>       {
>         rewind (f);
> -      if (fgets (buf, sizeof (buf), f) == NULL)
> -	{
> -	  printf ("fgets got %s.\n", strerror(errno));
> -	  lose = 1;
> -	}
> -      else if (strcmp (buf, replace))
> +      xfgets (buf, sizeof (buf), f);
> +      if (strcmp (buf, replace))
>   	{
>   	  printf ("Read \"%s\" instead of \"%s\".\n", buf, replace);
>   	  lose = 1;
> diff --git a/support/Makefile b/support/Makefile
> index 994639a915..c81e3c928c 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -123,6 +123,7 @@ libsupport-routines = \
>     xdup2 \
>     xfchmod \
>     xfclose \
> +  xfgets \
>     xfopen \
>     xfork \
>     xfread \
> diff --git a/support/xfgets.c b/support/xfgets.c
> new file mode 100644
> index 0000000000..14f98dee1b
> --- /dev/null
> +++ b/support/xfgets.c
> @@ -0,0 +1,32 @@
> +/* fgets with error checking.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/xstdio.h>
> +
> +#include <support/check.h>
> +#include <stdlib.h>
> +
> +char *
> +xfgets (char *s, int size, FILE *stream)
> +{
> +  char *ret = fgets (s, size, stream);
> +  if (!ret && ferror(stream))
> +    FAIL_EXIT1 ("fgets failed: %m");
> +
> +  return ret;
> +}
> diff --git a/support/xstdio.h b/support/xstdio.h
> index 633c342c82..f30bee6a20 100644
> --- a/support/xstdio.h
> +++ b/support/xstdio.h
> @@ -28,6 +28,7 @@ FILE *xfopen (const char *path, const char *mode);
>   void xfclose (FILE *);
>   FILE *xfreopen (const char *path, const char *mode, FILE *stream);
>   void xfread (void *ptr, size_t size, size_t nmemb, FILE *stream);
> +char *xfgets (char *s, int size, FILE *stream);
>   
>   /* Read a line from FP, using getline.  *BUFFER must be NULL, or a
>      heap-allocated pointer of *LENGTH bytes.  Return the number of
> diff --git a/sysdeps/pthread/tst-cancel6.c b/sysdeps/pthread/tst-cancel6.c
> index 63e6d49707..49b7399353 100644
> --- a/sysdeps/pthread/tst-cancel6.c
> +++ b/sysdeps/pthread/tst-cancel6.c
> @@ -20,12 +20,13 @@
>   #include <stdlib.h>
>   #include <unistd.h>
>   
> +#include <support/xstdio.h>
>   
>   static void *
>   tf (void *arg)
>   {
>     char buf[100];
> -  fgets (buf, sizeof (buf), arg);
> +  xfgets (buf, sizeof (buf), arg);
>     /* This call should never return.  */
>     return NULL;
>   }
  

On Tue, 13 Jun 2023, Siddhesh Poyarekar wrote:

> > diff --git a/assert/test-assert-perr.c b/assert/test-assert-perr.c
> > index 8496db6ffd..09a4fcb6ef 100644
> > --- a/assert/test-assert-perr.c
> > +++ b/assert/test-assert-perr.c
> > @@ -11,6 +11,8 @@
> >   #include <string.h>
> >   #include <setjmp.h>
> >   +#include <support/xstdio.h>
> > +
> >   jmp_buf rec;
> >   char buf[160];
> >   @@ -70,15 +72,15 @@ main(void)
> >       failed = 1; /* should not happen */
> >       rewind (stderr);
> > -  fgets (buf, 160, stderr);
> > +  xfgets (buf, 160, stderr);
> 
> Joseph, would you apply the same rationale for these tests too, i.e. since the
> test involves interaction with stdio and signals, would you avoid adding an
> xstdio abstraction here to test stdio directly?

I think this test is using fgets to check file contents rather than 
testing particular things about how fgets behaves.

> Likewise for the stdio-common test here and in patch 3/4, what do you think?

And those look they are using fread to check contents while testing some 
other stdio functions.
  

On 2023-06-13 14:25, Joseph Myers wrote:
> On Tue, 13 Jun 2023, Siddhesh Poyarekar wrote:
> 
>>> diff --git a/assert/test-assert-perr.c b/assert/test-assert-perr.c
>>> index 8496db6ffd..09a4fcb6ef 100644
>>> --- a/assert/test-assert-perr.c
>>> +++ b/assert/test-assert-perr.c
>>> @@ -11,6 +11,8 @@
>>>    #include <string.h>
>>>    #include <setjmp.h>
>>>    +#include <support/xstdio.h>
>>> +
>>>    jmp_buf rec;
>>>    char buf[160];
>>>    @@ -70,15 +72,15 @@ main(void)
>>>        failed = 1; /* should not happen */
>>>        rewind (stderr);
>>> -  fgets (buf, 160, stderr);
>>> +  xfgets (buf, 160, stderr);
>>
>> Joseph, would you apply the same rationale for these tests too, i.e. since the
>> test involves interaction with stdio and signals, would you avoid adding an
>> xstdio abstraction here to test stdio directly?
> 
> I think this test is using fgets to check file contents rather than
> testing particular things about how fgets behaves.
> 
>> Likewise for the stdio-common test here and in patch 3/4, what do you think?
> 
> And those look they are using fread to check contents while testing some
> other stdio functions.

OK, in that case, LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>