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).