[Support] fix TempFile infinite loop and permission denied errors

[inglorion]

Summary

On Windows, TempFile::create() was prone to failing with permission
denied errors when a process created many tempfiles without providing
a model large enough to accommodate them. There was also a problem
with createUniqueEntity getting into an infinite loop when all names
permitted by the model are in use. This change fixes both of these
problems and adds a unit test for them.

[zturner]

How could 16 characters not be enough? That’s 35^16 unique file names?

[inglorion]

@zturner, the models we use here are 6 characters. Chosen from a set of 16 characters gives 16M possibilities. Randomly generating them gives a 50%+ probability of a collision after 5000 names or so. I have another patch up (D50127) to use a 32-character set instead, which means the same 6 character model gives about a billion possible names and you need to generate about 40,000 of them before you have a 50% probability of a collision.

[inglorion]

This diff fixes a problem where, on Windows, TempFile::create() would immediately fail when the same name is tried again before the first file with that name is gone. That makes collisions less of an issue, because we can actually use more of the available names before we give up. It also fixes a problem where trying to create a tempfile when all available names are taken would lead to an infinite loop.

[inglorion]

Is this good to go?

[pcc]

No, I have comments.

[pcc]

llvm/lib/Support/Path.cpp

194 ↗

(On Diff #158454)

This is unlikely to be the problem given the number of % characters typically in a model. It's more likely to be the case that we're getting permission denied because we don't actually have permission. That's what I'd say in the comment here.

[inglorion]

Sorry, Peter. Saw your reply after I had already pushed the change. Would:

// Limit the number of attempts we make, so that we don't infinite loop. E.g.
// "permission denied" could be for a specific file (so we retry with a
// different name) or for the whole directory (retry would always fail).
// Checking which is racy, so we try a number of times, then give up.

be a better comment? If so, I'll land that as a follow-up.

[pcc]

Looks great, thanks.

[inglorion]

Comment changed in rL338755.

[fedor.sergeev]

The test introduced here seems to be invalid.
It assumes that 16*128 attempts to choose a random hex number will cover all 16 available numbers.
Obviously, the chance for this test to fail is rather low but it is not 0.

(and yes, today it failed in my test run)

[inglorion]

@fedor.sergeev: You're right, thanks for pointing that out. Sent D56336 to fix that.

llvm/trunk/lib/Support/Path.cpp

createUniqueEntity(const Twine &Model, int &ResultFD,
 
   // From here on, DO NOT modify model. It may be needed if the randomly chosen
   // path already exists.
   ResultPath = ModelStorage;
   // Null terminate.
   ResultPath.push_back(0);
   ResultPath.pop_back();
 
-retry_random_path:
+  // Limit the number of attempts we make, so that we don't infinite loop when
+  // we run out of filenames that fit the model.
+  std::error_code EC;
+  for (int Retries = 128; Retries > 0; --Retries) {
     // Replace '%' with random chars.
     for (unsigned i = 0, e = ModelStorage.size(); i != e; ++i) {
       if (ModelStorage[i] == '%')
-      ResultPath[i] = "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
+        ResultPath[i] =
+            "0123456789abcdef"[sys::Process::GetRandomNumber() & 15];
     }
 
     // Try to open + create the file.
     switch (Type) {
     case FS_File: {
-    if (std::error_code EC =
-            sys::fs::openFileForReadWrite(Twine(ResultPath.begin()), ResultFD,
-                                          sys::fs::CD_CreateNew, Flags, Mode)) {
-      if (EC == errc::file_exists)
-        goto retry_random_path;
+      EC = sys::fs::openFileForReadWrite(Twine(ResultPath.begin()), ResultFD,
+                                         sys::fs::CD_CreateNew, Flags, Mode);
+      if (EC) {
+        // errc::permission_denied happens on Windows when we try to open a file
+        // that has been marked for deletion.
+        if (EC == errc::file_exists || EC == errc::permission_denied)
+          continue;
         return EC;
       }
 
       return std::error_code();
     }
 
     case FS_Name: {
-    std::error_code EC =
-        sys::fs::access(ResultPath.begin(), sys::fs::AccessMode::Exist);
+      EC = sys::fs::access(ResultPath.begin(), sys::fs::AccessMode::Exist);
       if (EC == errc::no_such_file_or_directory)
         return std::error_code();
       if (EC)
         return EC;
-    goto retry_random_path;
+      continue;
     }
 
     case FS_Dir: {
-    if (std::error_code EC =
-            sys::fs::create_directory(ResultPath.begin(), false)) {
+      EC = sys::fs::create_directory(ResultPath.begin(), false);
+      if (EC) {
         if (EC == errc::file_exists)
-        goto retry_random_path;
+          continue;
         return EC;
       }
       return std::error_code();
     }
     }
     llvm_unreachable("Invalid Type");
   }
+  return EC;
+}
 
 namespace llvm {
 namespace sys  {
 namespace path {
 
 const_iterator begin(StringRef path, Style style) {
   const_iterator i;
   i.Path      = path;

llvm/trunk/unittests/Support/Path.cpp

const char *Path216 =
     "abcdefghijklmnopqrstuvwxyz5abcdefghijklmnopqrstuvwxyz4"
     "abcdefghijklmnopqrstuvwxyz3abcdefghijklmnopqrstuvwxyz2"
     "abcdefghijklmnopqrstuvwxyz1abcdefghijklmnopqrstuvwxyz0";
   ASSERT_NO_ERROR(fs::createTemporaryFile(Path216, "", TempPath));
   ASSERT_NO_ERROR(fs::remove(Twine(TempPath)));
 #endif
 }
 
+TEST_F(FileSystemTest, TempFileCollisions) {
+  SmallString<128> TestDirectory;
+  ASSERT_NO_ERROR(
+      fs::createUniqueDirectory("CreateUniqueFileTest", TestDirectory));
+  FileRemover Cleanup(TestDirectory);
+  SmallString<128> Model = TestDirectory;
+  path::append(Model, "%.tmp");
+  SmallString<128> Path;
+  std::vector<fs::TempFile> TempFiles;
+
+  auto TryCreateTempFile = [&]() {
+    Expected<fs::TempFile> T = fs::TempFile::create(Model);
+    if (T) {
+      TempFiles.push_back(std::move(*T));
+      return true;
+    } else {
+      logAllUnhandledErrors(T.takeError(), errs(),
+                            "Failed to create temporary file: ");
+      return false;
+    }
+  };
+
+  // We should be able to create exactly 16 temporary files.
+  for (int i = 0; i < 16; ++i)
+    EXPECT_TRUE(TryCreateTempFile());
+  EXPECT_FALSE(TryCreateTempFile());
+
+  for (fs::TempFile &T : TempFiles)
+    cantFail(T.discard());
+}
+
 TEST_F(FileSystemTest, CreateDir) {
   ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory) + "foo"));
   ASSERT_NO_ERROR(fs::create_directory(Twine(TestDirectory) + "foo"));
   ASSERT_EQ(fs::create_directory(Twine(TestDirectory) + "foo", false),
             errc::file_exists);
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "foo"));
 
 #ifdef LLVM_ON_UNIX