diff mbox series

[v2] fgets: more tests

Message ID xnikvwlzkc.fsf@greed.delorie.com
State New
Headers show
Series [v2] fgets: more tests | expand

Commit Message

DJ Delorie Aug. 20, 2024, 2:04 a.m. UTC
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

Comments

Florian Weimer Sept. 2, 2024, 3:20 p.m. UTC | #1
* 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
diff mbox series

Patch

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index a91754f52d..30c9637e72 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..e0127317bb
--- /dev/null
+++ b/stdio-common/tst-fgets2.c
@@ -0,0 +1,360 @@ 
+/* 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 <libc-diag.h>
+DIAG_PUSH_NEEDS_COMMENT;
+/* We're intentionally passing an invalid size and/or NULL later.  */
+DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
+#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")
+
+/*------------------------------------------------------------*/
+/* 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", __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 ("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));
+    }
+  else if (bytes_read != 6)
+    {
+      FAIL ("%d bytes read instead of %d", bytes_read, 6);
+    }
+  else
+    PASS ();
+  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 ("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);
+
+  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);
+
+  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);
+
+  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);
+    }
+  else
+    PASS ();
+  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 ("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);
+
+  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 ("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);
+
+#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 ("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 ();
+
+  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 ("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 ();
+#endif
+
+  return 0;
+}
+
+#include <support/test-driver.c>