Re: [PATCH] libstdc++: testsuite: avoid predictable mkstemp

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4690 bytes --]

On Thu, 23 Jun 2022 at 12:39, Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> > On Wed, 22 Jun 2022 at 07:05, Alexandre Oliva via Libstdc++
> > <libstdc++@gcc.gnu.org> wrote:
>
> >> It was prompted by a target system with a non-random implementation of
> >> mkstemp, that returns a predictable sequence of filenames and selects
> >> the first one that isn't already taken.
>
> > OK
>
> And here's the patch that enabled me to stop worrying about the above.
> Regstrapped on x86_64-linux-gnu, also tested with a cross to
> aarch64-rtems6.  Ok to install?
>
>
> __gnu_test::nonexistent_path: Always include counter in filename returned
>
> From: Joel Brobecker <brobecker@adacore.com>
>
> We have noticed that, on RTEMS, a small number of testscases are
> failing because two calls to this method return the same filename.
> This happens for instance in 27_io/filesystem/operations/copy_file.cc
> where it does:
>
>   auto from = __gnu_test::nonexistent_path();
>   auto to = __gnu_test::nonexistent_path();
>
> We tracked this issue down to the fact that the implementation of
> mkstemp on that system appears to use a very predictable algorithm
> for chosing the name of the temporary file, where the same filename
> appears to be tried in the same order, regardless of past calls.
> So, as long as the file gets deleted after a call to mkstemp (something
> we do here in our nonexistent_path method), the next call to mkstemps
> ends up returning the same filename, causing the collision we se above.
>
> This commit enhances the __gnu_test::nonexistent_path method to
> introduce in the filename being returned a counter which gets
> incremented at every call of this method.
>
> libstdc++-v3/ChangeLog:
>
>         * testsuite/util/testsuite_fs.h (__gnu_test::nonexistent_path):
>         Always include a counter in the filename returned.
> ---
>  libstdc++-v3/testsuite/util/testsuite_fs.h |   31 ++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h
> index 037d9ffc0f429..206ea67779003 100644
> --- a/libstdc++-v3/testsuite/util/testsuite_fs.h
> +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h
> @@ -38,9 +38,9 @@ namespace test_fs = std::experimental::filesystem;
>
>  #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L
>  #include <stdlib.h> // mkstemp
> -#else
> -#include <random>   // std::random_device
> +#include <cstring> // strcpy
>  #endif
> +#include <random>   // std::random_device
>
>  #if defined(__MINGW32__) || defined(__MINGW64__) \
>    || !defined (_GLIBCXX_HAVE_SYMLINK)
> @@ -125,8 +125,32 @@ namespace __gnu_test
>        file.erase(0, pos+1);
>
>      test_fs::path p;
> +    // A counter, starting from a random value, to be included as part
> +    // of the filename being returned, and incremented each time
> +    // this method is used.  It allows us to ensure that two calls
> +    // to this method can never return the same filename, something
> +    // testcases do when they need multiple non-existent filenames
> +    // for their purposes.
> +    static unsigned counter = std::random_device{}();
> +
>  #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L
> -    char tmp[] = "filesystem-test.XXXXXX";
> +    // Use mkstemp to determine the name of a file which does not exist yet.
> +    //
> +    // Note that we have seen on some systems (such as RTEMS, for instance)
> +    // that mkstemp behaves very predictably, causing it to always try
> +    // the same sequence of file names.  In other words, if we call mkstemp
> +    // with a pattern, delete the file it created (which is what we do, here),
> +    // and call mkstemp with the same pattern again, it returns the same
> +    // filename once more.  While most implementations introduce a degree
> +    // of randomness, it is not mandated by the standard, and this is why
> +    // we include a counter in the template passed to mkstemp.
> +    std::string mkstemp_template ("filesystem-test.");
> +    mkstemp_template.append(std::to_string (counter++));
> +    mkstemp_template.append(".XXXXXX");
> +
> +    char tmp[mkstemp_template.length() + 1];
> +    std::strcpy (tmp, mkstemp_template.c_str());

std::string::copy can be used to copy a string into a buffer, but
there's no reason to use another buffer anyway.

The attached makes this a bit more efficient, and makes more of the
code common to the mkstemp and non-mkstmp branches. I'll wait to hear
back from you before pushing it (since it has Joel's name on the
patch).

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4808 bytes --]

commit 833337d861e3126f6304e8bc9d8e743022a21424
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu Jun 23 13:12:12 2022

    libstdc++: testsuite: avoid predicable mkstemp
    
    We have noticed that, on RTEMS, a small number of testscases are
    failing because two calls to this method return the same filename.
    This happens for instance in 27_io/filesystem/operations/copy_file.cc
    where it does:
    
      auto from = __gnu_test::nonexistent_path();
      auto to = __gnu_test::nonexistent_path();
    
    We tracked this issue down to the fact that the implementation of
    mkstemp on that system appears to use a very predictable algorithm
    for chosing the name of the temporary file, where the same filename
    appears to be tried in the same order, regardless of past calls.
    So, as long as the file gets deleted after a call to mkstemp (something
    we do here in our nonexistent_path method), the next call to mkstemps
    ends up returning the same filename, causing the collision we se above.
    
    This commit enhances the __gnu_test::nonexistent_path method to
    introduce in the filename being returned a counter which gets
    incremented at every call of this method.
    
    Co-authored-by: Jonathan Wakely <jwakely@redhat.com>
    
    libstdc++-v3/ChangeLog:
    
            * testsuite/util/testsuite_fs.h (__gnu_test::nonexistent_path):
            Always include a counter in the filename returned.

diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h
index 9358a04e56c..3a197c70a1e 100644
--- a/libstdc++-v3/testsuite/util/testsuite_fs.h
+++ b/libstdc++-v3/testsuite/util/testsuite_fs.h
@@ -32,14 +32,14 @@ namespace test_fs = std::experimental::filesystem;
 #endif
 #include <algorithm>
 #include <fstream>
+#include <random>   // std::random_device
 #include <string>
+#include <system_error>
 #include <cstdio>
 #include <unistd.h> // unlink, close, getpid, geteuid
 
 #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L
 #include <stdlib.h> // mkstemp
-#else
-#include <random>   // std::random_device
 #endif
 
 namespace __gnu_test
@@ -109,32 +109,51 @@ namespace __gnu_test
     if (pos != file.npos)
       file.erase(0, pos+1);
 
+    file.reserve(file.size() + 40);
+    file.insert(0, "filesystem-test.");
+
+    // A counter, starting from a random value, to be included as part
+    // of the filename being returned, and incremented each time
+    // this function is used.  It allows us to ensure that two calls
+    // to this function can never return the same filename, something
+    // testcases do when they need multiple non-existent filenames
+    // for their purposes.
+    static unsigned counter = std::random_device{}();
+    file += '.';
+    file += std::to_string(counter++);
+    file += '.';
+
     test_fs::path p;
 #if defined(_GNU_SOURCE) || _XOPEN_SOURCE >= 500 || _POSIX_C_SOURCE >= 200112L
-    char tmp[] = "filesystem-test.XXXXXX";
-    int fd = ::mkstemp(tmp);
+
+    // Use mkstemp to determine the name of a file which does not exist yet.
+    //
+    // Note that we have seen on some systems (such as RTEMS, for instance)
+    // that mkstemp behaves very predictably, causing it to always try
+    // the same sequence of file names.  In other words, if we call mkstemp
+    // with a pattern, delete the file it created (which is what we do, here),
+    // and call mkstemp with the same pattern again, it returns the same
+    // filename once more.  While most implementations introduce a degree
+    // of randomness, it is not mandated by the standard, and this is why
+    // we also include a counter in the template passed to mkstemp.
+    file += "XXXXXX";
+    int fd = ::mkstemp(&file[0]);
     if (fd == -1)
       throw test_fs::filesystem_error("mkstemp failed",
 	  std::error_code(errno, std::generic_category()));
-    ::unlink(tmp);
+    ::unlink(file.c_str());
     ::close(fd);
-    if (!file.empty())
-      file.insert(0, 1, '-');
-    file.insert(0, tmp);
-    p = file;
+    p = std::move(file);
 #else
     if (file.length() > 64)
       file.resize(64);
-    char buf[128];
-    static unsigned counter = std::random_device{}();
-#if _GLIBCXX_USE_C99_STDIO
-    std::snprintf(buf, 128,
-#else
-    std::sprintf(buf,
-#endif
-      "filesystem-test.%u.%lu-%s", counter++, (unsigned long) ::getpid(),
-      file.c_str());
-    p = buf;
+    // The combination of random counter and PID should be unique for a given
+    // run of the testsuite.
+    file += std::to_string(::getpid());
+    p = std::move(file);
+    if (test_fs::exists(p))
+      throw test_fs::filesystem_error("Failed to generate unique pathname", p,
+	  std::make_error_code(std::errc::file_exists));
 #endif
     return p;
   }