[v4,06/13] misc: FUSE-based tests for mkstemp

  The tests check that O_EXCL is used properly, that 0600 is used
as the mode, that the characters used are as expected, and that
the distribution of names generated is reasonably random.

The tests run very slowly on some kernel versions, so make them
xtests.
---
 misc/Makefile                    |   6 +
 misc/tst-mkstemp-fuse-parallel.c | 208 +++++++++++++++++++++++++++++++
 misc/tst-mkstemp-fuse.c          | 195 +++++++++++++++++++++++++++++
 3 files changed, 409 insertions(+)
 create mode 100644 misc/tst-mkstemp-fuse-parallel.c
 create mode 100644 misc/tst-mkstemp-fuse.c
  

A few minor fixups needed but mostly OK.

* comments needed in some spots
* type changes
* atomics questions
* range checks

Florian Weimer <fweimer@redhat.com> writes:
> The tests check that O_EXCL is used properly, that 0600 is used
> as the mode, that the characters used are as expected, and that
> the distribution of names generated is reasonably random.
>
> The tests run very slowly on some kernel versions, so make them
> xtests.
> ---
>  misc/Makefile                    |   6 +
>  misc/tst-mkstemp-fuse-parallel.c | 208 +++++++++++++++++++++++++++++++
>  misc/tst-mkstemp-fuse.c          | 195 +++++++++++++++++++++++++++++
>  3 files changed, 409 insertions(+)
>  create mode 100644 misc/tst-mkstemp-fuse-parallel.c
>  create mode 100644 misc/tst-mkstemp-fuse.c


> diff --git a/misc/Makefile b/misc/Makefile
> +# Tests with long run times.
> +xtests += \
> +  tst-mkstemp-fuse \
> +  tst-mkstemp-fuse-parallel \
> +  # xtests

Ok.

> diff --git a/misc/tst-mkstemp-fuse-parallel.c b/misc/tst-mkstemp-fuse-parallel.c
> +/* FUSE-based test for mkstemp.  Parallel collision statistics.
> +   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/>.  */
> +
> +#include <stdlib.h>
> +
> +#include <array_length.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/fuse.h>
> +#include <support/support.h>
> +#include <support/xthread.h>
> +#include <support/xunistd.h>

Ok.

> +/* Enough space to record 4 times the expected number of replies (62**3).
> +   See the processes array in do_test.  */
> +enum { results_allocated = 4 * 62 * 62 * 62 + 1000 };

I don't see how the 1000 relates.  62*62*62 is the minimum number of
attempts in try_tempname_len but the upper limit is 238328.

> +/* The thread will store the results there.  */
> +static uint64_t *results;

Does it need to be volatile?  Or should atomics be used to access it?

> +/* Currently used part of the results array.  */
> +static uint64_t results_used;

Ok but same comment as above.

> +/* Fail with EEXIST, but record observed names.  */
> +static void
> +fuse_thread (struct support_fuse *f, void *closure)
> +{
> +  struct fuse_in_header *inh;
> +  while ((inh = support_fuse_next (f)) != NULL)
> +    {
> +      if (support_fuse_handle_mountpoint (f)
> +          || (inh->nodeid == 1 && support_fuse_handle_directory (f)))
> +        continue;
> +      if (inh->opcode != FUSE_LOOKUP || results_used >= results_allocated)
> +        {
> +          support_fuse_reply_error (f, EIO);
> +          continue;
> +        }

Ok.

> +      char *name = support_fuse_cast (LOOKUP, inh);
> +      TEST_COMPARE_BLOB (name, 3, "new", 3);
> +      TEST_COMPARE (strlen (name), 9);
> +      /* Extract 8 bytes of the name: 'w', the X replacements, and the
> +         null terminator.  */
> +      memcpy (&results[results_used], name + 2, 8);

results[] should thus be an array of char, not an array of int64_t
(not that it matters).  Or a comment should be here explaining that
we're coersing this into an integer for comparison purposes.

> +      ++results_used;

Protect against overflow?

> +      struct fuse_entry_out *out = support_fuse_prepare_entry (f, 2);
> +      out->attr.mode = S_IFREG | 0600;
> +      support_fuse_reply_prepared (f);
> +    }
> +}

It looks like we're reporting that the file was found, but if the file
is always found, how does mkstemp find an unused entry?  I think we need
a comment about how this action relates to the test below.

> +/* Used to sort the results array, to find duplicates.  */
> +static int
> +results_sort (const void *a1, const void *b1)
> +{
> +  const uint64_t *a = a1;
> +  const uint64_t *b = b1;
> +  if (*a < *b)
> +    return -1;
> +  if (*a == *b)
> +    return 0;
> +  return 1;
> +}

Ok.

> +/* Number of occurrences of certain streak lengths.  */
> +static size_t streak_lengths[6];

Ok.

> +/* Called for every encountered streak.  */
> +static inline void
> +report_streak (uint64_t current, size_t length)
> +{
> +  if (length > 1)
> +    {
> +      printf ("info: name \"ne%.8s\" repeats: %zu\n",
> +              (char *) &current, length);
> +      TEST_VERIFY_EXIT (length < array_length (streak_lengths));
> +    }
> +  ++streak_lengths[length];
> +}

Need to protect against length >= 6

> +static int
> +do_test (void)
> +{
> +  support_fuse_init ();
> +
> +  results = xmalloc (results_allocated * sizeof (*results));

Ok

> +  pid_t processes[4];
> +
> +  struct shared
> +  {
> +    /* Used to synchronize the start of all subprocesses, to make it
> +       more likely to expose concurrency-related bugs.  */
> +    pthread_barrier_t barrier1;
> +    pthread_barrier_t barrier2;
> +
> +    /* Filled in after fork.  */
> +    char mountpoint[PATH_MAX];
> +  };
> +
> +  /* Used to synchronize the start of all subprocesses, to make it
> +     more likely to expose concurrency-related bugs.  */
> +  struct shared *pshared = support_shared_allocate (sizeof (*pshared));
> +  {
> +    pthread_barrierattr_t attr;
> +    xpthread_barrierattr_init (&attr);
> +    xpthread_barrierattr_setpshared (&attr, PTHREAD_PROCESS_SHARED);
> +    xpthread_barrierattr_destroy (&attr);
> +    xpthread_barrier_init (&pshared->barrier1, &attr,
> +                           array_length (processes) + 1);
> +    xpthread_barrier_init (&pshared->barrier2, &attr,
> +                           array_length (processes) + 1);
> +    xpthread_barrierattr_destroy (&attr);
> +  }

Ok.

> +  for (int i = 0; i < array_length (processes); ++i)
> +    {
> +      processes[i] = xfork ();
> +      if (processes[i] == 0)
> +        {
> +          /* Wait for mountpoint initialization.  */
> +          xpthread_barrier_wait (&pshared->barrier1);
> +          char *path = xasprintf ("%s/newXXXXXX", pshared->mountpoint);
> +
> +          /* Park this process until all processes have started.  */
> +          xpthread_barrier_wait (&pshared->barrier2);
> +          errno = 0;
> +          TEST_COMPARE (mkstemp (path), -1);
> +          TEST_COMPARE (errno, EEXIST);
> +          free (path);
> +          _exit (0);
> +        }
> +    }

Each thread calls mkstemp once, which tries up to... 238328? times to
create a unique filename... ok

> +  /* Do this after the forking, to minimize initialization inteference.  */
> +  struct support_fuse *f = support_fuse_mount (fuse_thread, NULL);
> +  TEST_VERIFY (strlcpy (pshared->mountpoint, support_fuse_mountpoint (f),
> +                        sizeof (pshared->mountpoint))
> +               < sizeof (pshared->mountpoint));
> +  xpthread_barrier_wait (&pshared->barrier1);

Ok.

> +  puts ("info: performing mkstemp calls");
> +  xpthread_barrier_wait (&pshared->barrier2);

Ok.

> +  for (int i = 0; i < array_length (processes); ++i)
> +    {
> +      int status;
> +      xwaitpid (processes[i], &status, 0);
> +      TEST_COMPARE (status, 0);
> +    }
o
Ok.
> +  support_fuse_unmount (f);

Ok.

> +  puts ("info: checking results");
> +  qsort (results, results_used, sizeof (*results), results_sort);
> +
> +  size_t current = -1;

This should be uint64_t not size_t

> +  size_t streak = 0;
> +  for (size_t i = 0; i < results_used; ++i)
> +    if (results[i] == current)
> +      ++streak;
> +    else
> +      {
> +        report_streak (current, streak);
> +        current = results[i];
> +        streak = 1;
> +      }
> +  report_streak (current, streak);

Ok.

> +  puts ("info: repetition count distribution:");
> +  for (int i = 1; i < array_length (streak_lengths); ++i)
> +    printf ("  length %d: %zu\n", i, streak_lengths[i]);

Ok.

> +  /* Some arbitrary threshold, hopefully unlikely enough.  Note that
> +     these thresholds are considerably lower than what uniformly
> +     distributed file names, chosen independently, would produce.  */
> +  if (streak_lengths[2] > 20)
> +    FAIL ("unexpectedly common repetition count 2: %zu", streak_lengths[2]);
> +  if (streak_lengths[3] > 1)
> +    FAIL ("unexpectedly common repetition count 3: %zu", streak_lengths[3]);
> +  for (int i = 4; i < array_length (streak_lengths); ++i)
> +    if (streak_lengths[i] > 0)
> +      FAIL ("too many repeats of count %d: %zu", i, streak_lengths[i]);
> +
> +  return 0;
> +}
> +#include <support/test-driver.c>

Ok

> diff --git a/misc/tst-mkstemp-fuse.c b/misc/tst-mkstemp-fuse.c
> +/* FUSE-based test for mkstemp.
> +   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/>.  */
> +
> +#include <stdlib.h>
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/fuse.h>
> +#include <support/support.h>
> +#include <support/xunistd.h>

Ok.

> +/* Set to true in do_test to cause the first FUSE_CREATE attempt to fail.  */
> +static _Atomic bool simulate_creat_race;

Ok.

> +/* Basic tests with eventually successful creation.  */
> +static void
> +fuse_thread_basic (struct support_fuse *f, void *closure)
> +{
> +  char *previous_name = NULL;
> +  int state = 0;
> +  struct fuse_in_header *inh;
> +  while ((inh = support_fuse_next (f)) != NULL)
> +    {
> +      if (support_fuse_handle_mountpoint (f)
> +          || (inh->nodeid == 1 && support_fuse_handle_directory (f)))
> +        continue;
> +
> +      switch (inh->opcode)
> +        {
> +        case FUSE_LOOKUP:
> +          /* File does not exist initially.  */
> +          TEST_COMPARE (inh->nodeid, 1);
> +          if (simulate_creat_race)

Atomic read?

> +            {
> +              if (state < 3)
> +                ++state;
> +              else
> +                FAIL ("invalid state: %d", state);
> +            }
> +          else
> +            {
> +              TEST_COMPARE (state, 0);
> +              state = 3;
> +            }
> +          support_fuse_reply_error (f, ENOENT);
> +          break;

Ok.

> +        case FUSE_CREATE:
> +          {
> +            TEST_COMPARE (inh->nodeid, 1);
> +            char *name;
> +            struct fuse_create_in *p
> +              = support_fuse_cast_name (CREATE, inh, &name);
> +            /* Name follows after struct fuse_create_in.  */
> +            TEST_COMPARE (p->flags & O_ACCMODE, O_RDWR);
> +            TEST_VERIFY (p->flags & O_EXCL);
> +            TEST_VERIFY (p->flags & O_CREAT);
> +            TEST_COMPARE (p->mode & 07777, 0600);
> +            TEST_VERIFY (S_ISREG (p->mode));
> +            TEST_COMPARE_BLOB (name, 3, "new", 3);

Ok.

> +            if (state != 3 && simulate_creat_race)
> +              {
> +                ++state;
> +                support_fuse_reply_error (f, EEXIST);
> +              }
> +            else
> +              {
> +                if (previous_name != NULL)
> +                  /* This test has a very small probability of failure
> +                     due to a harmless collision (one in 62**6 tests).  */
> +                  TEST_VERIFY (strcmp (name, previous_name) != 0);
> +                TEST_COMPARE (state, 3);
> +                ++state;
> +                struct fuse_entry_out *entry;
> +                struct fuse_open_out *open;
> +                support_fuse_prepare_create (f, 2, &entry, &open);
> +                entry->attr.mode = S_IFREG | 0600;
> +                support_fuse_reply_prepared (f);
> +              }
> +            free (previous_name);
> +            previous_name = xstrdup (name);
> +          }
> +          break;

Ok.

> +        case FUSE_FLUSH:
> +        case FUSE_RELEASE:
> +          TEST_COMPARE (state, 4);
> +          TEST_COMPARE (inh->nodeid, 2);
> +          support_fuse_reply_empty (f);
> +          break;
> +        default:
> +          support_fuse_reply_error (f, EIO);
> +        }
> +    }
> +  free (previous_name);
> +}

Ok.

> +/* Reply that all files exist.  */
> +static void
> +fuse_thread_eexist (struct support_fuse *f, void *closure)
> +{
> +  uint64_t counter = 0;
> +  struct fuse_in_header *inh;
> +  while ((inh = support_fuse_next (f)) != NULL)
> +    {
> +      if (support_fuse_handle_mountpoint (f)
> +          || (inh->nodeid == 1 && support_fuse_handle_directory (f)))
> +        continue;
> +
> +      switch (inh->opcode)
> +        {
> +        case FUSE_LOOKUP:
> +          ++counter;
> +          TEST_COMPARE (inh->nodeid, 1);
> +          char *name = support_fuse_cast (LOOKUP, inh);
> +          TEST_COMPARE_BLOB (name, 3, "new", 3);
> +          TEST_COMPARE (strlen (name), 9);
> +          for (int i = 3; i <= 8; ++i)
> +            {
> +              /* The glibc implementation uses letters and digits only.  */
> +              char ch = name[i];
> +              TEST_VERIFY (('0' <= ch && ch <= '9')
> +                           || ('a' <= ch && ch <= 'z')
> +                           || ('A' <= ch && ch <= 'Z'));
> +            }
> +          struct fuse_entry_out out =
> +            {
> +              .nodeid = 2,
> +              .attr = {
> +                .mode = S_IFREG | 0600,
> +                .ino = 2,
> +              },
> +            };
> +          support_fuse_reply (f, &out, sizeof (out));
> +          break;
> +        default:
> +          support_fuse_reply_error (f, EIO);
> +        }
> +    }
> +  TEST_VERIFY (counter >= 200000);

Comment needed - where does the 200000 come from?

> +}

Ok.

> +static int
> +do_test (void)
> +{
> +  support_fuse_init ();
> +
> +  for (int do_simulate_creat_race = 0; do_simulate_creat_race < 2;
> +       ++do_simulate_creat_race)
> +    {
> +      simulate_creat_race = do_simulate_creat_race;

Atomic write?

> +      printf ("info: testing with simulate_creat_race == %d\n",
> +              (int) simulate_creat_race);
> +      struct support_fuse *f = support_fuse_mount (fuse_thread_basic, NULL);
> +      char *path = xasprintf ("%s/newXXXXXX", support_fuse_mountpoint (f));
> +      int fd = mkstemp (path);
> +      TEST_VERIFY (fd > 2);
> +      xclose (fd);
> +      free (path);
> +      support_fuse_unmount (f);
> +    }

Ok.

> +  puts ("info: testing EEXIST failure case for mkstemp");
> +  {
> +    struct support_fuse *f = support_fuse_mount (fuse_thread_eexist, NULL);
> +    char *path = xasprintf ("%s/newXXXXXX", support_fuse_mountpoint (f));
> +    errno = 0;
> +    TEST_COMPARE (mkstemp (path), -1);
> +    TEST_COMPARE (errno, EEXIST);
> +    free (path);
> +    support_fuse_unmount (f);
> +  }

Ok.

> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.
  

* DJ Delorie:

>> +/* Enough space to record 4 times the expected number of replies (62**3).
>> +   See the processes array in do_test.  */
>> +enum { results_allocated = 4 * 62 * 62 * 62 + 1000 };
>
> I don't see how the 1000 relates.  62*62*62 is the minimum number of
> attempts in try_tempname_len but the upper limit is 238328.

I just wanted to add some slack.  I can drop the 1000.

>> +/* The thread will store the results there.  */
>> +static uint64_t *results;
>
> Does it need to be volatile?  Or should atomics be used to access it?

This one (misc/tst-mkstemp-fuse-parallel) doesn't need _Atomic because
the thread is started after initialization and stopped before the
gathered data is interpreted.

>> +      char *name = support_fuse_cast (LOOKUP, inh);
>> +      TEST_COMPARE_BLOB (name, 3, "new", 3);
>> +      TEST_COMPARE (strlen (name), 9);
>> +      /* Extract 8 bytes of the name: 'w', the X replacements, and the
>> +         null terminator.  */
>> +      memcpy (&results[results_used], name + 2, 8);
>
> results[] should thus be an array of char, not an array of int64_t
> (not that it matters).  Or a comment should be here explaining that
> we're coersing this into an integer for comparison purposes.

I switched to:

      /* Extract 8 bytes of the name: 'w', the X replacements, and the
         null terminator.  Treat it as an uint64_t for easy sorting
         below.  Endianess does not matter because the relative order
         of the entries is not important; sorting is only used to find
         duplicates.  */

>> +      ++results_used;
>
> Protect against overflow?

Added:

      ++results_used;
      TEST_VERIFY_EXIT (results_used != 0); /* Check for overflow.  */

>> +      struct fuse_entry_out *out = support_fuse_prepare_entry (f, 2);
>> +      out->attr.mode = S_IFREG | 0600;
>> +      support_fuse_reply_prepared (f);
>> +    }
>> +}
>
> It looks like we're reporting that the file was found, but if the file
> is always found, how does mkstemp find an unused entry?  I think we need
> a comment about how this action relates to the test below.

I changed the function comment to:

/* Fail with EEXIST (so that mkstemp tries again).  Record observed
   names for later statistical analysis.  */

>> +/* Called for every encountered streak.  */
>> +static inline void
>> +report_streak (uint64_t current, size_t length)
>> +{
>> +  if (length > 1)
>> +    {
>> +      printf ("info: name \"ne%.8s\" repeats: %zu\n",
>> +              (char *) &current, length);
>> +      TEST_VERIFY_EXIT (length < array_length (streak_lengths));
>> +    }
>> +  ++streak_lengths[length];
>> +}
>
> Need to protect against length >= 6

Fixed:

  TEST_VERIFY_EXIT (length < array_length (streak_lengths));
  ++streak_lengths[length];

>> +  puts ("info: checking results");
>> +  qsort (results, results_used, sizeof (*results), results_sort);
>> +
>> +  size_t current = -1;
>
> This should be uint64_t not size_t

Fixed.

>> +/* Set to true in do_test to cause the first FUSE_CREATE attempt to fail.  */
>> +static _Atomic bool simulate_creat_race;


>> +      switch (inh->opcode)
>> +        {
>> +        case FUSE_LOOKUP:
>> +          /* File does not exist initially.  */
>> +          TEST_COMPARE (inh->nodeid, 1);
>> +          if (simulate_creat_race)
>
> Atomic read?

It's implicit due to the _Atomic.

>> +    }
>> +  TEST_VERIFY (counter >= 200000);
>
> Comment needed - where does the 200000 come from?

Added:

  /* Verify that mkstemp has retried a lot.  The current
     implementation tries 62 * 62 * 62 times until it goves up.  */
  TEST_VERIFY (counter >= 200000);

Thanks,
Florian