| Message ID | orv8st9qt6.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: testsuite: avoid predictable mkstemp | expand |
On Wed, 22 Jun 2022 at 07:05, Alexandre Oliva via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > > This patch was originally meant to reduce the likelihood that > nonexistent_path() returns the same pathname for from and to. > > 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. > > That turned out not to be enough: nonexistent_path adds a suffix to > the filename chosen by mkstemp and removes the file it created, so > mkstemp may very well insist on the same basename, and the case that > doesn't use mkstemp doesn't even check whether the file already > exists. > > Anyway, by the time I realized this wasn't enough, I'd already > implemented some of the changes, and I figured I might as well > contribute them, even though they don't really solve any problem, and > even if they did, they'd be just a partial solution. > > Regstrapped on x86_64-linux-gnu, also tested with a cross to > aarch64-rtems6. Ok to install? OK
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()); + int fd = ::mkstemp(tmp); if (fd == -1) throw test_fs::filesystem_error("mkstemp failed", @@ -141,7 +165,6 @@ namespace __gnu_test 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
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). 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; }
On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > 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). Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it works for us.
On Mon, 27 Jun 2022 at 10:32, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > 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). > > Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it > works for us. I've pushed it to trunk now.
On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: >> 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). > Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it > works for us. The bad news is that it broke on some other systems I didn't test back then. It turns out the type cast for the ::getpid result was not just because it was passed to printf before :-/ libstdc++: testsuite: cast getpid result On vxworks, in kernel mode, getpid's return type is a pointer type, so std::to_string on it fails overload resolution. Restore the type cast from the original patch that suggested adding the pid. Regstrapped on x86_64-linux-gnu, also tested on aarch64-rtems6.0 and ppc64-vx7r2. I'm going ahead and checking this in as obvious. Please let me know if you'd prefer this to be fixed in a different way. for libstdc++/ChangeLog * testsuite/util/testsuite_fs.h (nonexistent_path): Convert the getpid result to an integral type. --- libstdc++-v3/testsuite/util/testsuite_fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h index 908fcdbcaeed1..25f8f734dc792 100644 --- a/libstdc++-v3/testsuite/util/testsuite_fs.h +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h @@ -163,7 +163,7 @@ namespace __gnu_test file.resize(64); // The combination of random counter and PID should be unique for a given // run of the testsuite. - file += std::to_string(::getpid()); + file += std::to_string((unsigned long) ::getpid()); p = std::move(file); if (test_fs::exists(p)) throw test_fs::filesystem_error("Failed to generate unique pathname", p,
On Tue, 5 Jul 2022 at 10:10, Alexandre Oliva via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > > > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > >> 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). > > > Thanks, I've given it a spin, both trunk and gcc-11, and I confirm it > > works for us. > > The bad news is that it broke on some other systems I didn't test back > then. It turns out the type cast for the ::getpid result was not just > because it was passed to printf before :-/ Ah, whoops. I did check that POSIX requires pid_t to be a signed integer type, but that doesn't mean it's always true for all GCC targets. > > > libstdc++: testsuite: cast getpid result > > On vxworks, in kernel mode, getpid's return type is a pointer type, so > std::to_string on it fails overload resolution. Restore the type cast > from the original patch that suggested adding the pid. > > Regstrapped on x86_64-linux-gnu, also tested on aarch64-rtems6.0 and > ppc64-vx7r2. I'm going ahead and checking this in as obvious. Please > let me know if you'd prefer this to be fixed in a different way. The cast itself is fine, but I'd like a comment like "N.B. pid_t is a pointer on vxworks" so I don't "simplify" it again. Thanks.
On Jul 5, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > The cast itself is fine, but I'd like a comment like "N.B. pid_t is a > pointer on vxworks" so I don't "simplify" it again. libstdc++: testsuite: why cast getpid result Add a comment next to the getpid call to explain why the typecast is needed. Regstrapped on x86_64-linux-gnu, will install later today if there's no objection. for libstdc++-v3/ChangeLog * testsuite/util/testsuite_fs (nonexistent_path): Explain why we need the typecast. --- libstdc++-v3/testsuite/util/testsuite_fs.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/testsuite/util/testsuite_fs.h b/libstdc++-v3/testsuite/util/testsuite_fs.h index 25f8f734dc792..0e28385e99aca 100644 --- a/libstdc++-v3/testsuite/util/testsuite_fs.h +++ b/libstdc++-v3/testsuite/util/testsuite_fs.h @@ -162,7 +162,8 @@ namespace __gnu_test if (file.length() > 64) file.resize(64); // The combination of random counter and PID should be unique for a given - // run of the testsuite. + // run of the testsuite. N.B. getpid() returns a pointer type on vxworks + // in kernel mode. file += std::to_string((unsigned long) ::getpid()); p = std::move(file); if (test_fs::exists(p))
On Tue, 5 Jul 2022 at 18:46, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jul 5, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > The cast itself is fine, but I'd like a comment like "N.B. pid_t is a > > pointer on vxworks" so I don't "simplify" it again. > > > libstdc++: testsuite: why cast getpid result > > Add a comment next to the getpid call to explain why the typecast is > needed. > > Regstrapped on x86_64-linux-gnu, will install later today if there's no > objection. Thanks.
diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc index b936e04493b5c..f3081f4b64ebc 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc @@ -73,7 +73,6 @@ test02() const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec; ec = bad_ec; @@ -81,6 +80,7 @@ test02() VERIFY( !ec ); VERIFY( fs::exists(from) ); + auto to = __gnu_test::nonexistent_path(); ec = bad_ec; fs::copy(from, to, fs::copy_options::skip_symlinks, ec); VERIFY( !ec ); @@ -117,12 +117,13 @@ void test03() { auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); // test empty file std::ofstream{from}; VERIFY( fs::exists(from) ); VERIFY( fs::file_size(from) == 0 ); + + auto to = __gnu_test::nonexistent_path(); fs::copy(from, to); VERIFY( fs::exists(to) ); VERIFY( fs::file_size(to) == 0 ); @@ -145,11 +146,11 @@ test04() { const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec; create_directories(from/"a/b/c"); + auto to = __gnu_test::nonexistent_path(); { __gnu_test::scoped_file f(to); copy(from, to, ec); diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc index 5cd6b483c269b..ca38328c5da15 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc @@ -73,7 +73,6 @@ test02() #endif auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec, bad = std::make_error_code(std::errc::invalid_argument); ec = bad; @@ -81,6 +80,7 @@ test02() VERIFY( !ec ); VERIFY( fs::exists(from) ); + auto to = __gnu_test::nonexistent_path(); ec = bad; fs::copy(from, to, fs::copy_options::skip_symlinks, ec); VERIFY( !ec ); @@ -116,12 +116,13 @@ void test03() { auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); // test empty file std::ofstream{from.c_str()}; VERIFY( fs::exists(from) ); VERIFY( fs::file_size(from) == 0 ); + + auto to = __gnu_test::nonexistent_path(); fs::copy(from, to); VERIFY( fs::exists(to) ); VERIFY( fs::file_size(to) == 0 ); @@ -143,11 +144,11 @@ void test04() { auto from = __gnu_test::nonexistent_path(); - auto to = __gnu_test::nonexistent_path(); std::error_code ec; create_directories(from/"a/b/c"); + auto to = __gnu_test::nonexistent_path(); { __gnu_test::scoped_file f(to); copy(from, to, ec);