[v1] fgets: more tests

  make test:
PASS: stdio-common/tst-fgets2

*.out looks like:
testing base operation...PASS
testing zero size file...PASS
testing zero size buffer...PASS
testing NULL buffer...PASS
testing embedded NUL...PASS
testing writable stream...PASS
testing closed fd stream...PASS

I left in two more tests that check for valid FILE* parameters, but
those checks are conditional in the libc sources.  I made the tests
conditional the same way, although I don't see a way of enabling these
either in glibc or here.  The tests would otherwise just always segfault.

 - - - - -

Add more tests for unusual situations fgets() might see:

* zero size file
* zero sized buffer
* NULL buffer
* NUL data
* writable stream
* closed stream
  

On 09/08/24 16:05, DJ Delorie wrote:
> 
> make test:
> PASS: stdio-common/tst-fgets2
> 
> *.out looks like:
> testing base operation...PASS
> testing zero size file...PASS
> testing zero size buffer...PASS
> testing NULL buffer...PASS
> testing embedded NUL...PASS
> testing writable stream...PASS
> testing closed fd stream...PASS
> 
> I left in two more tests that check for valid FILE* parameters, but
> those checks are conditional in the libc sources.  I made the tests
> conditional the same way, although I don't see a way of enabling these
> either in glibc or here.  The tests would otherwise just always segfault.
> 
>  - - - - -
> 
> Add more tests for unusual situations fgets() might see:
> 
> * zero size file
> * zero sized buffer
> * NULL buffer
> * NUL data
> * writable stream
> * closed stream
> 

I has failed on Linaro CI [1].  To get the test output result, check the
tests.log.0.xz file, it contains all the failed tests outputs:

FAIL: stdio-common/tst-fgets2
original exit status 127

So it seems to be a crash on the test.

[1] https://ci.linaro.org/job/tcwg_glibc_check--master-arm-precommit/2321/artifact/artifacts/artifacts.precommit/00-sumfiles/

> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index e4f0146d2c..2c54badf3b 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -209,6 +209,7 @@ tests := \
>    tst-fdopen \
>    tst-ferror \
>    tst-fgets \
> +  tst-fgets2 \
>    tst-fileno \
>    tst-fmemopen \
>    tst-fmemopen2 \
> diff --git a/stdio-common/tst-fgets2.c b/stdio-common/tst-fgets2.c
> new file mode 100644
> index 0000000000..7a76b181c4
> --- /dev/null
> +++ b/stdio-common/tst-fgets2.c
> @@ -0,0 +1,354 @@
> +/* Test for additional fgets error handling.
> +   Copyright (C) 2024 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/>.  */
> +
> +#define _GNU_SOURCE 1
> +
> +#include <stdio.h>
> +#include <error.h>
> +#include <errno.h>
> +#include <limits.h>
> +#include <mcheck.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <ctype.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +#include <libc-diag.h>
> +
> +/*------------------------------------------------------------*/
> +/* Implementation of our FILE stream backend.  */
> +
> +static int bytes_read;
> +static int bytes_written;
> +static int cookie_valid = 0;
> +struct Cookie {
> +  const char *buffer;
> +  int bufptr;
> +  int bufsz;
> +};
> +
> +#define VALIDATE_COOKIE() if (! cookie_valid) { \
> +  FAIL ("call to %s after file closed\n", __FUNCTION__); \
> +  return -1;    \
> +  }
> +
> +static ssize_t
> +io_read (void *vcookie, char *buf, size_t size)
> +{
> +  struct Cookie *cookie = (struct Cookie *) vcookie;
> +
> +  VALIDATE_COOKIE ();
> +
> +  if (size > cookie->bufsz - cookie->bufptr)
> +    size = cookie->bufsz - cookie->bufptr;
> +
> +  memcpy (buf, cookie->buffer + cookie->bufptr, size);
> +  cookie->bufptr += size;
> +  bytes_read += size;
> +  return size;
> +}
> +
> +static ssize_t
> +io_write (void *vcookie, const char *buf, size_t size)
> +{
> +  VALIDATE_COOKIE ();
> +
> +  bytes_written += size;
> +  return -1;
> +}
> +
> +static int
> +io_seek (void *vcookie, off64_t *position, int whence)
> +{
> +  VALIDATE_COOKIE ();
> +  return -1;
> +}
> +
> +static int
> +io_clean (void *vcookie)
> +{
> +  struct Cookie *cookie = (struct Cookie *) vcookie;
> +
> +  VALIDATE_COOKIE ();
> +
> +  cookie->buffer = NULL;
> +  cookie->bufsz = 0;
> +  cookie->bufptr = 0;
> +
> +  cookie_valid = 0;
> +  free (cookie);
> +  return 0;
> +}
> +
> +cookie_io_functions_t io_funcs = {
> +  .read = io_read,
> +  .write = io_write,
> +  .seek = io_seek,
> +  .close = io_clean
> +};
> +
> +FILE *
> +io_open (const char *buffer, int buflen, const char *mode, void **vcookie)
> +{
> +  FILE *f;
> +  struct Cookie *cookie;
> +
> +  cookie = (struct Cookie *) xcalloc (1, sizeof (struct Cookie));
> +  *vcookie = cookie;
> +  cookie_valid = 1;
> +
> +  cookie->buffer = buffer;
> +  cookie->bufsz = buflen;
> +  bytes_read = 0;
> +  bytes_written = 0;
> +
> +  f = fopencookie (cookie, mode, io_funcs);
> +  if (f == NULL)
> +    {
> +      perror ("fopencookie");
> +      exit (1);
> +    }
> +
> +  return f;
> +}
> +
> +/*------------------------------------------------------------*/
> +/* Convenience functions.  */
> +
> +static char *
> +hex (const char *b, int s)
> +{
> +  static int bi = 0;
> +  static char buf[3][100];
> +  static char digit[] = "0123456789ABCDEF";
> +  char *bp;
> +  if (s > 30)
> +    exit (1);
> +  bi = (bi+1)%3;
> +  bp = buf[bi];
> +  while (s > 0)
> +    {
> +      if (isgraph (*b))
> +	*bp ++ = *b;
> +      else if (*b == '\n')
> +	{
> +	  *bp ++ = '\\';
> +	  *bp ++ = 'n';
> +	}
> +      else if (*b == '\0')
> +	{
> +	  *bp ++ = '\\';
> +	  *bp ++ = '0';
> +	}
> +      else
> +	{
> +	  *bp ++ = 'x';
> +	  *bp ++ = digit[(*b >> 4) & 0x0f];
> +	  *bp ++ = digit[*b & 0x0f];
> +	}
> +      b ++;
> +      s --;
> +    }
> +  *bp = 0;
> +  return buf[bi];
> +}
> +
> +#define my_open(s,l,m) io_open (s, l, m, (void *) &cookie)
> +
> +/*------------------------------------------------------------*/
> +/* The test cases.  */
> +
> +static int
> +do_test (void)
> +{
> +  FILE *f;
> +  struct Cookie *cookie;
> +  char buf [100];
> +  char *str;
> +
> +  printf ("testing base operation...");
> +  f = my_open ("hello\n", 6, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str == NULL)
> +    {
> +      FAIL ("FAIL (returned NULL)\n");
> +    }
> +  else if (memcmp (str, "hello\n\0", 7) != 0)
> +    {
> +      FAIL ("FAIL (returned %s instead of %s)\n", hex (str, 7), hex ("hello\n\0", 7));
> +    }
> +  else if (bytes_read != 6)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 6);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing zero size file...");
> +  f = my_open ("hello\n", 0, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing zero size buffer...");
> +  f = my_open ("hello\n", 6, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 0, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing NULL buffer...");
> +  f = my_open ("hello\n", 0, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +
> +  DIAG_PUSH_NEEDS_COMMENT;
> +  /* We're intentionally passing an invalid size here.  */
> +  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
> +  str = fgets (NULL, 100, f);
> +  DIAG_POP_NEEDS_COMMENT;
> +
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing embedded NUL...");
> +  f = my_open ("hel\0lo\n", 7, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str == NULL)
> +    {
> +      FAIL ("FAIL (returned NULL)\n");
> +    }
> +  else if (memcmp (str, "hel\0lo\n\0", 8) != 0)
> +    {
> +      FAIL ("FAIL (returned %s instead of %s)\n", hex (str, 8), hex ("hel\0lo\n\0", 8));
> +    }
> +  else if (bytes_read != 7)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 7);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing writable stream...");
> +  f = my_open ("hel\0lo\n", 7, "w");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing closed fd stream...");
> +  int fd = open ("/dev/null", O_RDONLY);
> +  f = fdopen (fd, "r");
> +  close (fd);
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +#ifdef IO_DEBUG
> +  /* These tests only pass if glibc is built with -DIO_DEBUG.  */
> +
> +  printf ("testing NULL descriptor...");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, NULL);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +
> +  printf ("testing closed descriptor...");
> +  f = my_open ("hello\n", 7, "r");
> +  fclose (f);
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +#endif
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>
  

Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
> I has failed on Linaro CI [1].  To get the test output result, check the
> tests.log.0.xz file, it contains all the failed tests outputs:
>
> FAIL: stdio-common/tst-fgets2
> original exit status 127
>
> So it seems to be a crash on the test.

The test logs disagree:

In file included from ../include/bits/stdio2.h:1,
                 from ../libio/stdio.h:970,
                 from ../include/stdio.h:14,
                 from tst-fgets2.c:21:
In function ‘fgets’,
    inlined from ‘do_test’ at tst-fgets2.c:248:9:
../libio/bits/stdio2.h:313:12: error: argument 1 is null but the corresponding size argument 2 value is 100 [-Werror=nonnull]
  313 |     return __fgets_alias (__s, __n, __stream);
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/sys/cdefs.h:10,
                 from ../include/features.h:511,
                 from ../bits/floatn-common.h:23,
                 from ../bits/floatn.h:52,
                 from ../include/stdio.h:7,
                 from tst-fgets2.c:21:
../libio/bits/stdio2.h: In function ‘do_test’:
../libio/bits/stdio2-decl.h:96:26: note: in a call to function ‘__fgets_alias’ declared with attribute ‘access (write_only, 1, 2)’
   96 | extern char *__REDIRECT (__fgets_alias,
      |                          ^~~~~~~~~~~~~
../misc/sys/cdefs.h:410:41: note: in definition of macro ‘__REDIRECT’
  410 | # define __REDIRECT(name, proto, alias) name proto __asm__ (__ASMNAME (#alias))
      |                                         ^~~~

However, that call is explicitly wrapped in macros that should ignore
that error:

  DIAG_PUSH_NEEDS_COMMENT;
  /* We're intentionally passing an invalid size here.  */
  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
  str = fgets (NULL, 100, f);
  DIAG_POP_NEEDS_COMMENT;

That silenced it locally.  I wonder why it doesn't silence it in your CI
chain?  Am I misunderstanding how these DIAG_* work?
  

On 13/08/24 01:12, DJ Delorie wrote:
> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>> I has failed on Linaro CI [1].  To get the test output result, check the
>> tests.log.0.xz file, it contains all the failed tests outputs:
>>
>> FAIL: stdio-common/tst-fgets2
>> original exit status 127
>>
>> So it seems to be a crash on the test.
> 
> The test logs disagree:
> 
> In file included from ../include/bits/stdio2.h:1,
>                  from ../libio/stdio.h:970,
>                  from ../include/stdio.h:14,
>                  from tst-fgets2.c:21:
> In function ‘fgets’,
>     inlined from ‘do_test’ at tst-fgets2.c:248:9:
> ../libio/bits/stdio2.h:313:12: error: argument 1 is null but the corresponding size argument 2 value is 100 [-Werror=nonnull]
>   313 |     return __fgets_alias (__s, __n, __stream);
>       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/sys/cdefs.h:10,
>                  from ../include/features.h:511,
>                  from ../bits/floatn-common.h:23,
>                  from ../bits/floatn.h:52,
>                  from ../include/stdio.h:7,
>                  from tst-fgets2.c:21:
> ../libio/bits/stdio2.h: In function ‘do_test’:
> ../libio/bits/stdio2-decl.h:96:26: note: in a call to function ‘__fgets_alias’ declared with attribute ‘access (write_only, 1, 2)’
>    96 | extern char *__REDIRECT (__fgets_alias,
>       |                          ^~~~~~~~~~~~~
> ../misc/sys/cdefs.h:410:41: note: in definition of macro ‘__REDIRECT’
>   410 | # define __REDIRECT(name, proto, alias) name proto __asm__ (__ASMNAME (#alias))
>       |                                         ^~~~
> 
> However, that call is explicitly wrapped in macros that should ignore
> that error:
> 
>   DIAG_PUSH_NEEDS_COMMENT;
>   /* We're intentionally passing an invalid size here.  */
>   DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>   str = fgets (NULL, 100, f);
>   DIAG_POP_NEEDS_COMMENT;
> 
> That silenced it locally.  I wonder why it doesn't silence it in your CI
> chain?  Am I misunderstanding how these DIAG_* work?

It seems something has changed on gcc 12, and the Linaro bots uses the 
system compiler which is gcc 11.  On gcc 11 it seems that the _Pragma (...)
only works for static inline if you add them *before* function declaration,
where on gcc 12 it works on function instantiation.

This following patch fixes the issues on older gcc:

diff --git a/stdio-common/tst-fgets2.c b/stdio-common/tst-fgets2.c
index 7a76b181c4..4d11a589a4 100644
--- a/stdio-common/tst-fgets2.c
+++ b/stdio-common/tst-fgets2.c
@@ -18,7 +18,14 @@

 #define _GNU_SOURCE 1

+#include <libc-diag.h>
+DIAG_PUSH_NEEDS_COMMENT;
+/* We're intentionally pass an invalid size here for fget for fgets.  The
+   warning disable requires to be before function declaration on gcc 11
+   or older.  */
+DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
 #include <stdio.h>
+DIAG_POP_NEEDS_COMMENT;
 #include <error.h>
 #include <errno.h>
 #include <limits.h>
@@ -33,7 +40,6 @@

 #include <support/support.h>
 #include <support/check.h>
-#include <libc-diag.h>

 /*------------------------------------------------------------*/
 /* Implementation of our FILE stream backend.  */


We still need to use DIAG_IGNORE_NEEDS_COMMENT on the function instantiation
to avoid gcc12 issues.  Another option would disable the warning for all
translation unit, to cover the fgets call as well:

#include <libc-diag.h>
DIAG_PUSH_NEEDS_COMMENT;
DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
#include <stdio.h>
[...]
  fgets (...)
DIAG_POP_NEEDS_COMMENT
  

On 8/9/24 3:05 PM, DJ Delorie wrote:
> 
> make test:
> PASS: stdio-common/tst-fgets2
> 
> *.out looks like:
> testing base operation...PASS
> testing zero size file...PASS
> testing zero size buffer...PASS
> testing NULL buffer...PASS
> testing embedded NUL...PASS
> testing writable stream...PASS
> testing closed fd stream...PASS

I like this... but it's unstructured?

> 
> I left in two more tests that check for valid FILE* parameters, but
> those checks are conditional in the libc sources.  I made the tests
> conditional the same way, although I don't see a way of enabling these
> either in glibc or here.  The tests would otherwise just always segfault.
> 
>  - - - - -
> 
> Add more tests for unusual situations fgets() might see:
> 
> * zero size file
> * zero sized buffer
> * NULL buffer
> * NUL data
> * writable stream
> * closed stream
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index e4f0146d2c..2c54badf3b 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -209,6 +209,7 @@ tests := \
>    tst-fdopen \
>    tst-ferror \
>    tst-fgets \
> +  tst-fgets2 \
>    tst-fileno \
>    tst-fmemopen \
>    tst-fmemopen2 \
> diff --git a/stdio-common/tst-fgets2.c b/stdio-common/tst-fgets2.c
> new file mode 100644
> index 0000000000..7a76b181c4
> --- /dev/null
> +++ b/stdio-common/tst-fgets2.c
> @@ -0,0 +1,354 @@
> +/* Test for additional fgets error handling.
> +   Copyright (C) 2024 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/>.  */
> +
> +#define _GNU_SOURCE 1
> +
> +#include <stdio.h>
> +#include <error.h>
> +#include <errno.h>
> +#include <limits.h>
> +#include <mcheck.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <ctype.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +#include <libc-diag.h>
> +
> +/*------------------------------------------------------------*/
> +/* Implementation of our FILE stream backend.  */
> +
> +static int bytes_read;
> +static int bytes_written;
> +static int cookie_valid = 0;
> +struct Cookie {
> +  const char *buffer;
> +  int bufptr;
> +  int bufsz;
> +};
> +
> +#define VALIDATE_COOKIE() if (! cookie_valid) { \
> +  FAIL ("call to %s after file closed\n", __FUNCTION__); \
> +  return -1;    \
> +  }
> +
> +static ssize_t
> +io_read (void *vcookie, char *buf, size_t size)
> +{
> +  struct Cookie *cookie = (struct Cookie *) vcookie;
> +
> +  VALIDATE_COOKIE ();
> +
> +  if (size > cookie->bufsz - cookie->bufptr)
> +    size = cookie->bufsz - cookie->bufptr;
> +
> +  memcpy (buf, cookie->buffer + cookie->bufptr, size);
> +  cookie->bufptr += size;
> +  bytes_read += size;
> +  return size;
> +}
> +
> +static ssize_t
> +io_write (void *vcookie, const char *buf, size_t size)
> +{
> +  VALIDATE_COOKIE ();
> +
> +  bytes_written += size;
> +  return -1;
> +}
> +
> +static int
> +io_seek (void *vcookie, off64_t *position, int whence)
> +{
> +  VALIDATE_COOKIE ();
> +  return -1;
> +}
> +
> +static int
> +io_clean (void *vcookie)
> +{
> +  struct Cookie *cookie = (struct Cookie *) vcookie;
> +
> +  VALIDATE_COOKIE ();
> +
> +  cookie->buffer = NULL;
> +  cookie->bufsz = 0;
> +  cookie->bufptr = 0;
> +
> +  cookie_valid = 0;
> +  free (cookie);
> +  return 0;
> +}
> +
> +cookie_io_functions_t io_funcs = {
> +  .read = io_read,
> +  .write = io_write,
> +  .seek = io_seek,
> +  .close = io_clean
> +};
> +
> +FILE *
> +io_open (const char *buffer, int buflen, const char *mode, void **vcookie)
> +{
> +  FILE *f;
> +  struct Cookie *cookie;
> +
> +  cookie = (struct Cookie *) xcalloc (1, sizeof (struct Cookie));
> +  *vcookie = cookie;
> +  cookie_valid = 1;
> +
> +  cookie->buffer = buffer;
> +  cookie->bufsz = buflen;
> +  bytes_read = 0;
> +  bytes_written = 0;
> +
> +  f = fopencookie (cookie, mode, io_funcs);
> +  if (f == NULL)
> +    {
> +      perror ("fopencookie");
> +      exit (1);
> +    }
> +
> +  return f;
> +}
> +
> +/*------------------------------------------------------------*/
> +/* Convenience functions.  */
> +
> +static char *
> +hex (const char *b, int s)
> +{
> +  static int bi = 0;
> +  static char buf[3][100];
> +  static char digit[] = "0123456789ABCDEF";
> +  char *bp;
> +  if (s > 30)
> +    exit (1);
> +  bi = (bi+1)%3;
> +  bp = buf[bi];
> +  while (s > 0)
> +    {
> +      if (isgraph (*b))
> +	*bp ++ = *b;
> +      else if (*b == '\n')
> +	{
> +	  *bp ++ = '\\';
> +	  *bp ++ = 'n';
> +	}
> +      else if (*b == '\0')
> +	{
> +	  *bp ++ = '\\';
> +	  *bp ++ = '0';
> +	}
> +      else
> +	{
> +	  *bp ++ = 'x';
> +	  *bp ++ = digit[(*b >> 4) & 0x0f];
> +	  *bp ++ = digit[*b & 0x0f];
> +	}
> +      b ++;
> +      s --;
> +    }
> +  *bp = 0;
> +  return buf[bi];
> +}
> +
> +#define my_open(s,l,m) io_open (s, l, m, (void *) &cookie)
> +
> +/*------------------------------------------------------------*/
> +/* The test cases.  */
> +
> +static int
> +do_test (void)
> +{
> +  FILE *f;
> +  struct Cookie *cookie;
> +  char buf [100];
> +  char *str;
> +
> +  printf ("testing base operation...");

I dislike that we are printing unstructured information about the test?

Can we put this into TEST_NAME ("testing base operation...")

And return a test handle?

> +  f = my_open ("hello\n", 6, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str == NULL)
> +    {
> +      FAIL ("FAIL (returned NULL)\n");

... this is linked to a sub-test result.

> +    }
> +  else if (memcmp (str, "hello\n\0", 7) != 0)
> +    {
> +      FAIL ("FAIL (returned %s instead of %s)\n", hex (str, 7), hex ("hello\n\0", 7));

... this is linked to a sub-test result.

> +    }
> +  else if (bytes_read != 6)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 6);

... this is linked to a sub-test result.

> +    }
> +  else
> +    printf ("PASS\n");

I dislike that we are printing unstructured PASS information to stdout.

Do we need sub-test counting for pass/fail in support/*?

I'd hope we could say PASS ("test name"); ?

The alternative is we modify TEST_VERIFY e.g.

TEST_VERIFY_PF (<assertion>,
                "pass message",
                "fail messsage")

> +  fclose (f);
> +
> +  printf ("testing zero size file...");
> +  f = my_open ("hello\n", 0, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);

Why doesn't this use FAIL?

> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing zero size buffer...");
> +  f = my_open ("hello\n", 6, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 0, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);

Why doesn't this use FAIL?

> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing NULL buffer...");
> +  f = my_open ("hello\n", 0, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +
> +  DIAG_PUSH_NEEDS_COMMENT;
> +  /* We're intentionally passing an invalid size here.  */
> +  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
> +  str = fgets (NULL, 100, f);
> +  DIAG_POP_NEEDS_COMMENT;
> +
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing embedded NUL...");
> +  f = my_open ("hel\0lo\n", 7, "r");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str == NULL)
> +    {
> +      FAIL ("FAIL (returned NULL)\n");
> +    }
> +  else if (memcmp (str, "hel\0lo\n\0", 8) != 0)
> +    {
> +      FAIL ("FAIL (returned %s instead of %s)\n", hex (str, 8), hex ("hel\0lo\n\0", 8));
> +    }
> +  else if (bytes_read != 7)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 7);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing writable stream...");
> +  f = my_open ("hel\0lo\n", 7, "w");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +  printf ("testing closed fd stream...");
> +  int fd = open ("/dev/null", O_RDONLY);
> +  f = fdopen (fd, "r");
> +  close (fd);
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +  fclose (f);
> +
> +#ifdef IO_DEBUG
> +  /* These tests only pass if glibc is built with -DIO_DEBUG.  */
> +
> +  printf ("testing NULL descriptor...");
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, NULL);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +
> +  printf ("testing closed descriptor...");
> +  f = my_open ("hello\n", 7, "r");
> +  fclose (f);
> +  memset (buf, 0x11, sizeof (buf));
> +  str = fgets (buf, 100, f);
> +  if (str != NULL)
> +    {
> +      FAIL ("FAIL (returned %s instead of NULL)\n", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
> +    }
> +  else
> +    printf ("PASS\n");
> +#endif
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>
  

On Tue, Aug 13, 2024 at 5:38 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 13/08/24 01:12, DJ Delorie wrote:
> > Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
> >> I has failed on Linaro CI [1].  To get the test output result, check the
> >> tests.log.0.xz file, it contains all the failed tests outputs:
> >>
> >> FAIL: stdio-common/tst-fgets2
> >> original exit status 127
> >>
> >> So it seems to be a crash on the test.
> >
> > The test logs disagree:
> >
> > In file included from ../include/bits/stdio2.h:1,
> >                  from ../libio/stdio.h:970,
> >                  from ../include/stdio.h:14,
> >                  from tst-fgets2.c:21:
> > In function ‘fgets’,
> >     inlined from ‘do_test’ at tst-fgets2.c:248:9:
> > ../libio/bits/stdio2.h:313:12: error: argument 1 is null but the corresponding size argument 2 value is 100 [-Werror=nonnull]
> >   313 |     return __fgets_alias (__s, __n, __stream);
> >       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In file included from ../include/sys/cdefs.h:10,
> >                  from ../include/features.h:511,
> >                  from ../bits/floatn-common.h:23,
> >                  from ../bits/floatn.h:52,
> >                  from ../include/stdio.h:7,
> >                  from tst-fgets2.c:21:
> > ../libio/bits/stdio2.h: In function ‘do_test’:
> > ../libio/bits/stdio2-decl.h:96:26: note: in a call to function ‘__fgets_alias’ declared with attribute ‘access (write_only, 1, 2)’
> >    96 | extern char *__REDIRECT (__fgets_alias,
> >       |                          ^~~~~~~~~~~~~
> > ../misc/sys/cdefs.h:410:41: note: in definition of macro ‘__REDIRECT’
> >   410 | # define __REDIRECT(name, proto, alias) name proto __asm__ (__ASMNAME (#alias))
> >       |                                         ^~~~
> >
> > However, that call is explicitly wrapped in macros that should ignore
> > that error:
> >
> >   DIAG_PUSH_NEEDS_COMMENT;
> >   /* We're intentionally passing an invalid size here.  */
> >   DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
> >   str = fgets (NULL, 100, f);
> >   DIAG_POP_NEEDS_COMMENT;
> >
> > That silenced it locally.  I wonder why it doesn't silence it in your CI
> > chain?  Am I misunderstanding how these DIAG_* work?
>
> It seems something has changed on gcc 12, and the Linaro bots uses the
> system compiler which is gcc 11.  On gcc 11 it seems that the _Pragma (...)
> only works for static inline if you add them *before* function declaration,
> where on gcc 12 it works on function instantiation.
>
> This following patch fixes the issues on older gcc:

This seems not related to the version of GCC but rather if
_FORTIFY_SOURCE is defined or not.

Thanks,
Andrew

>
> diff --git a/stdio-common/tst-fgets2.c b/stdio-common/tst-fgets2.c
> index 7a76b181c4..4d11a589a4 100644
> --- a/stdio-common/tst-fgets2.c
> +++ b/stdio-common/tst-fgets2.c
> @@ -18,7 +18,14 @@
>
>  #define _GNU_SOURCE 1
>
> +#include <libc-diag.h>
> +DIAG_PUSH_NEEDS_COMMENT;
> +/* We're intentionally pass an invalid size here for fget for fgets.  The
> +   warning disable requires to be before function declaration on gcc 11
> +   or older.  */
> +DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>  #include <stdio.h>
> +DIAG_POP_NEEDS_COMMENT;
>  #include <error.h>
>  #include <errno.h>
>  #include <limits.h>
> @@ -33,7 +40,6 @@
>
>  #include <support/support.h>
>  #include <support/check.h>
> -#include <libc-diag.h>
>
>  /*------------------------------------------------------------*/
>  /* Implementation of our FILE stream backend.  */
>
>
> We still need to use DIAG_IGNORE_NEEDS_COMMENT on the function instantiation
> to avoid gcc12 issues.  Another option would disable the warning for all
> translation unit, to cover the fgets call as well:
>
> #include <libc-diag.h>
> DIAG_PUSH_NEEDS_COMMENT;
> DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
> #include <stdio.h>
> [...]
>   fgets (...)
> DIAG_POP_NEEDS_COMMENT
  

On 13/08/24 13:26, Andrew Pinski wrote:
> On Tue, Aug 13, 2024 at 5:38 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 13/08/24 01:12, DJ Delorie wrote:
>>> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>>>> I has failed on Linaro CI [1].  To get the test output result, check the
>>>> tests.log.0.xz file, it contains all the failed tests outputs:
>>>>
>>>> FAIL: stdio-common/tst-fgets2
>>>> original exit status 127
>>>>
>>>> So it seems to be a crash on the test.
>>>
>>> The test logs disagree:
>>>
>>> In file included from ../include/bits/stdio2.h:1,
>>>                  from ../libio/stdio.h:970,
>>>                  from ../include/stdio.h:14,
>>>                  from tst-fgets2.c:21:
>>> In function ‘fgets’,
>>>     inlined from ‘do_test’ at tst-fgets2.c:248:9:
>>> ../libio/bits/stdio2.h:313:12: error: argument 1 is null but the corresponding size argument 2 value is 100 [-Werror=nonnull]
>>>   313 |     return __fgets_alias (__s, __n, __stream);
>>>       |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> In file included from ../include/sys/cdefs.h:10,
>>>                  from ../include/features.h:511,
>>>                  from ../bits/floatn-common.h:23,
>>>                  from ../bits/floatn.h:52,
>>>                  from ../include/stdio.h:7,
>>>                  from tst-fgets2.c:21:
>>> ../libio/bits/stdio2.h: In function ‘do_test’:
>>> ../libio/bits/stdio2-decl.h:96:26: note: in a call to function ‘__fgets_alias’ declared with attribute ‘access (write_only, 1, 2)’
>>>    96 | extern char *__REDIRECT (__fgets_alias,
>>>       |                          ^~~~~~~~~~~~~
>>> ../misc/sys/cdefs.h:410:41: note: in definition of macro ‘__REDIRECT’
>>>   410 | # define __REDIRECT(name, proto, alias) name proto __asm__ (__ASMNAME (#alias))
>>>       |                                         ^~~~
>>>
>>> However, that call is explicitly wrapped in macros that should ignore
>>> that error:
>>>
>>>   DIAG_PUSH_NEEDS_COMMENT;
>>>   /* We're intentionally passing an invalid size here.  */
>>>   DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>>>   str = fgets (NULL, 100, f);
>>>   DIAG_POP_NEEDS_COMMENT;
>>>
>>> That silenced it locally.  I wonder why it doesn't silence it in your CI
>>> chain?  Am I misunderstanding how these DIAG_* work?
>>
>> It seems something has changed on gcc 12, and the Linaro bots uses the
>> system compiler which is gcc 11.  On gcc 11 it seems that the _Pragma (...)
>> only works for static inline if you add them *before* function declaration,
>> where on gcc 12 it works on function instantiation.
>>
>> This following patch fixes the issues on older gcc:
> 
> This seems not related to the version of GCC but rather if
> _FORTIFY_SOURCE is defined or not.

Off course it is due _FORTIFY_SOURCE, otherwise there is not static inline wrapper
for fgets.  But the test uses exactly the same flags, and the _Pragma (...) to
suppress the warning only works at function instantiation with gcc 12.
  

"Carlos O'Donell" <carlos@redhat.com> writes:
> I like this... but it's unstructured?

It's debug info for the *.out file, just there in case the test fails
for someone but is unreproducible, the data in the *.out file might help
fix the bug.  Did you want more structure in the syntax (for parsing the
*.out files), or in the way it's generated?

>> +  printf ("testing base operation...");
>
> I dislike that we are printing unstructured information about the test?
>
> Can we put this into TEST_NAME ("testing base operation...")
>
> And return a test handle?

For what purpose?  Keeping track of counts of tests/fails, only to write
them in a file that nobody will look at? ;-)

>> +  f = my_open ("hello\n", 6, "r");
>> +  memset (buf, 0x11, sizeof (buf));
>> +  str = fgets (buf, 100, f);
>> +  if (str == NULL)
>> +    {
>> +      FAIL ("FAIL (returned NULL)\n");
>
> ... this is linked to a sub-test result.
>
>> +    }
>> +  else if (memcmp (str, "hello\n\0", 7) != 0)
>> +    {
>> +      FAIL ("FAIL (returned %s instead of %s)\n", hex (str, 7), hex ("hello\n\0", 7));
>
> ... this is linked to a sub-test result.
>
>> +    }
>> +  else if (bytes_read != 6)
>> +    {
>> +      FAIL ("FAIL (%d bytes read instead of %d)\n", bytes_read, 6);
>
> ... this is linked to a sub-test result.
>
>> +    }
>> +  else
>> +    printf ("PASS\n");
>
> I dislike that we are printing unstructured PASS information to stdout.

Note that the first printf doesn't have a \n so this goes on the same
line as the test name.

Is there any harm in mentioning that a subtest passed?

> Do we need sub-test counting for pass/fail in support/*?
>
> I'd hope we could say PASS ("test name"); ?

If we used that paradigm for the fails, a segfaulting test wouldn't
print the test name before truncating the *.out file (assuming stdout is
unbuffered).  So we can't use it for the pass's either.

If the idea is to collect metrics about sub-tests, I have to ask "for
who?"  That's a lot of work for only some of the tests, for a file
nobody will look at unless the test fails...

But to see what it would look like, I added a PASS() macro to
tst-fgets2.c, and replaced "PASS" with "ok" to avoid confusion in the
output (which prints "error" for FAIL()).  So you get a *.out file like
this:

testing base operation...ok
testing zero size file...ok
testing zero size buffer...error: tst-fgets2.c:238: 7 bytes read instead of 0
testing NULL buffer...ok
testing embedded NUL...ok
testing writable stream...ok
testing closed fd stream...ok

> The alternative is we modify TEST_VERIFY e.g.
>
> TEST_VERIFY_PF (<assertion>,
>                 "pass message",
>                 "fail messsage")

The problem with the VERIFY() macros is that they print the expression
being verified, not the data being used by the expression.  I.e. having
this in a *.out file is not that useful:

FAIL: i != j

But something like this is much more useful:

FAIL: found 0x45 not same as expected 0x46

It's very hard to macro-ize this type of output as the format depends on
the type of data and expression being tested.

>> +  else if (bytes_read != 0)
>> +    {
>> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
>
> Why doesn't this use FAIL?

Fixed.  I tweaked the message to fit better with what FAIL() emits.

>> +  else if (bytes_read != 0)
>> +    {
>> +      printf ("FAIL (%d bytes read instead of %d)\n", bytes_read, 0);
>
> Why doesn't this use FAIL?

Fixed.

Andrew Pinski <pinskia@gmail.com> wrote:
> This seems not related to the version of GCC but rather if
> _FORTIFY_SOURCE is defined or not.

I'm using gcc 11, and added -D_FORTIFY_SOURCE=2 to the build, but
couldn't reproduce this.  Nevertheless, I wrapped the includes in the
macros in hopes to fix it.

V2 to follow, hopefully CI will like it better.