[v2] fgets: more tests

  changes since v1:
* wrapped includes in DIAG_*
* a few printf->FAIL
* reformat FAIL messages
* macroize PASSes

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

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

* DJ Delorie:

> diff --git a/stdio-common/tst-fgets2.c b/stdio-common/tst-fgets2.c
> new file mode 100644
> index 0000000000..e0127317bb
> --- /dev/null
> +++ b/stdio-common/tst-fgets2.c

> +#define _GNU_SOURCE 1

Should be unnecessary?  All tests are built with _GNU_SOURCE by default,
I think?

> +#include <libc-diag.h>
> +DIAG_PUSH_NEEDS_COMMENT;
> +/* We're intentionally passing an invalid size and/or NULL later.  */
> +DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");

I'm surprised this works.  Shouldn't it be around the site of the
invalid use?

> +#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>
> +DIAG_POP_NEEDS_COMMENT;
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +
> +#define PASS() printf("ok\n")

Missing space before second '('.

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

We generally do not use ----- separators?

> +static ssize_t
> +io_write (void *vcookie, const char *buf, size_t size)
> +{
> +  VALIDATE_COOKIE ();
> +
> +  bytes_written += size;
> +  return -1;
> +}

Should this simply call FAIL_EXIT1 because it's not expected to be used?

> +static int
> +io_seek (void *vcookie, off64_t *position, int whence)
> +{
> +  VALIDATE_COOKIE ();
> +  return -1;
> +}

Likewise.

> +/*------------------------------------------------------------*/
> +/* Convenience functions.  */
> +
> +static char *
> +hex (const char *b, int s)

Wouldn't TEST_COMPARE_BLOB provide this for you?

> +#define my_open(s,l,m) io_open (s, l, m, (void *) &cookie)

Could be a regular function?


> +/*------------------------------------------------------------*/
> +/* 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 ("returned NULL");
> +    }
> +  else if (memcmp (str, "hello\n\0", 7) != 0)
> +    {
> +      FAIL ("returned %s instead of %s", hex (str, 7), hex ("hello\n\0", 7));

Could this check a bit further, to check that the 0x11 bytes are still
there?  Or isn't this a guarantee we want to provide?

> +  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 ("returned %s instead of NULL", hex (str, strlen (str)));
> +    }

Likewise, verify that buffer contents is unchanged?
Also check the stream error indicator.

> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("%d bytes read instead of %d", bytes_read, 0);
> +    }
> +  else
> +    PASS ();
> +  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 ("returned %s instead of NULL", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("%d bytes read instead of %d", bytes_read, 0);
> +    }
> +  else
> +    PASS ();
> +  fclose (f);

Buffer modification check, and check the stream error indicator?

> +
> +  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 ("returned %s instead of NULL", hex (str, strlen (str)));
> +    }
> +  else if (bytes_read != 0)
> +    {
> +      FAIL ("%d bytes read instead of %d", bytes_read, 0);
> +    }
> +  else
> +    PASS ();
> +  fclose (f);

I think you should use a compiler barrier (perhaps:
void *volatile null = NULL;) instead of disabling the warning.
Getting rid of the warning won't change code generation if GCC
replaces this with a trap (which future versions might).

> +
> +  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 ("returned NULL");
> +    }
> +  else if (memcmp (str, "hel\0lo\n\0", 8) != 0)
> +    {
> +      FAIL ("returned %s instead of %s", hex (str, 8), hex ("hel\0lo\n\0", 8));
> +    }
> +  else if (bytes_read != 7)
> +    {
> +      FAIL ("%d bytes read instead of %d", bytes_read, 7);
> +    }

Again, verifying unchanged bytes?


> +#ifdef IO_DEBUG
> +  /* These tests only pass if glibc is built with -DIO_DEBUG.  */

We don't build with IO_DEBUG, so these tests do not seem valuable to me,
sorry.

Thanks,
Florian
  

Florian Weimer <fweimer@redhat.com> writes:
>> +#define _GNU_SOURCE 1
>
> Should be unnecessary?  All tests are built with _GNU_SOURCE by default,
> I think?

I needed it at one point, but no longer.  Removed.

>> +#include <libc-diag.h>
>> +DIAG_PUSH_NEEDS_COMMENT;
>> +/* We're intentionally passing an invalid size and/or NULL later.  */
>> +DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>
> I'm surprised this works.  Shouldn't it be around the site of the
> invalid use?

It's *also* at the use site.  This is a workaround for building with
__FORTIFY_SOURCE=2 with at least gcc 11.

>> +#define PASS() printf("ok\n")
>
> Missing space before second '('.

Fixed.

>> +/*------------------------------------------------------------*/
>> +/* Implementation of our FILE stream backend.  */
>
> We generally do not use ----- separators?

We generally don't, no, but we should use some sort of separators
between groups of functionally-related code.

>> +static ssize_t
>> +io_write (void *vcookie, const char *buf, size_t size)
>> +{
>> +  VALIDATE_COOKIE ();
>> +
>> +  bytes_written += size;
>> +  return -1;
>> +}
>
> Should this simply call FAIL_EXIT1 because it's not expected to be used?

Could.  I was setting up a template for other similar tests, which would
check for that error by looking at bytes_written.

It would be better if the implementation of fopencookie allowed for NULL
functions, and did the Right Thing for us.

>> +/*------------------------------------------------------------*/
>> +/* Convenience functions.  */
>> +
>> +static char *
>> +hex (const char *b, int s)
>
> Wouldn't TEST_COMPARE_BLOB provide this for you?

Could.  It doesn't have an option for adding a message to the output,
but I can rename the relevent variables to be more self-documenting when
used by that.

However, TEST_COMPARE_BLOB does not print anything when it passes, nor
does it return a value that says if it passes.  This means a successful
test prints nothing, and one goal here is to be more verbose in noting
passing sub-tests.

>> +#define my_open(s,l,m) io_open (s, l, m, (void *) &cookie)
>
> Could be a regular function?

It wouldn't have access to the cookie.

>> +  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 ("returned NULL");
>> +    }
>> +  else if (memcmp (str, "hello\n\0", 7) != 0)
>> +    {
>> +      FAIL ("returned %s instead of %s", hex (str, 7), hex ("hello\n\0", 7));
>
> Could this check a bit further, to check that the 0x11 bytes are still
> there?  Or isn't this a guarantee we want to provide?

That's a good idea.

>> +  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 ("returned %s instead of NULL", hex (str, strlen (str)));
>> +    }
>
> Likewise, verify that buffer contents is unchanged?
> Also check the stream error indicator.

Ok.

>> +  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;
>
> I think you should use a compiler barrier (perhaps:
> void *volatile null = NULL;) instead of disabling the warning.
> Getting rid of the warning won't change code generation if GCC
> replaces this with a trap (which future versions might).

Done.

>> +#ifdef IO_DEBUG
>> +  /* These tests only pass if glibc is built with -DIO_DEBUG.  */
>
> We don't build with IO_DEBUG, so these tests do not seem valuable to me,
> sorry.

True, but they demonstrate that I thought of checking those cases, and
specifically couldn't, and the conditions that would allow testing them.