[Support][Windows] Make sure only executables are found by sys::findProgramByName

[zero9178]

Summary

The function utilizes Windows' SearchPathW function, which as I found out today, may also return directories. After looking at the Unix implementation of the file I found that it contains a check whether the found path is also executable. While fixing the Windows implementation, I also learned that sys::fs::access returns successfully when querying whether directories are executable, which the Unix version does not.

This patch makes both of these functions equivalent to their Unix implementation and insures that any path returned by sys::findProgramByName on Windows may only be executable, just like the Unix implementation.

The equivalent additions I have made to the Windows implementation, in the Unix implementation are here:
sys::findProgramByName: https://github.com/llvm/llvm-project/blob/39ecfe614350fa5db7b8f13f81212f8e3831a390/llvm/lib/Support/Unix/Program.inc#L90
sys::fs::access: https://github.com/llvm/llvm-project/blob/c2a84771bb63947695ea50b89160c02b36fb634d/llvm/lib/Support/Unix/Path.inc#L608

I encountered this issue when running the LLVM testsuite. Commands of the form not test ... would fail to correctly execute test.exe, which is part of GnuWin32, as it actually tried to execute a folder called test, which happened to be in a directory on my PATH.

Regarding tests:
I added a test to check that sys::fs::access returns permission_denied for a directory. Besides that I am unsure how to test sys::findProgramByName, nor the not utility.

[rnk]

lgtm

[amccarth]

Just some drive-by comments.

llvm/lib/Support/Windows/Program.inc

65

Adding the empty string to the list of possible extensions for an executable seems like an odd thing to do on Windows (and that's a big part of the reason why SearchPathW is finding directories). I guess that's attempting to handle the case where Name already has the extension (e.g., "foo.exe"). In that case, I think the empty string should be conditional on whether the Name already has something that looks like an extension.

68

As the FIXME suggests, adding ".exe" is unnecessary. Worse it changes the search order. The PATHEXT environment variable determines not only what extensions are considered executable, but also what order they should be searched. Usually .COM and .BAT should be found before .EXE.

I would have done:

C++
if (const char *PathExtEnv = std::getenv("PATHEXT"))
    SplitString(PathExtEnv, PathExts, ";");
else
    PathExts.push_back(".exe");

llvm/lib/Support/Windows/Path.inc

if (LastError != ERROR_FILE_NOT_FOUND &&
         LastError != ERROR_PATH_NOT_FOUND)
       return mapWindowsError(LastError);
     return errc::no_such_file_or_directory;
   }
 
   if (Mode == AccessMode::Write && (Attributes & FILE_ATTRIBUTE_READONLY))
     return errc::permission_denied;
 
+  if (Mode == AccessMode::Execute && (Attributes & FILE_ATTRIBUTE_DIRECTORY))
+    return errc::permission_denied;
+
   return std::error_code();
 }
 
 bool can_execute(const Twine &Path) {
   return !access(Path, AccessMode::Execute) ||
          !access(Path + ".exe", AccessMode::Execute);
 }
 

llvm/lib/Support/Windows/Program.inc

if (!Paths.empty()) {
     Path = PathStorage.c_str();
   }
 
   SmallVector<wchar_t, MAX_PATH> U16Name;
   if (std::error_code EC = windows::UTF8ToUTF16(Name, U16Name))
     return EC;
 
   SmallVector<StringRef, 12> PathExts;
   PathExts.push_back("");
   PathExts.push_back(".exe"); // FIXME: This must be in %PATHEXT%.
   if (const char *PathExtEnv = std::getenv("PATHEXT"))
     SplitString(PathExtEnv, PathExts, ";");
 
+  SmallVector<char, MAX_PATH> U8Result;
+  for (StringRef Ext : PathExts) {
     SmallVector<wchar_t, MAX_PATH> U16Result;
     DWORD Len = MAX_PATH;
-  for (StringRef Ext : PathExts) {
-    SmallVector<wchar_t, MAX_PATH> U16Ext;
-    if (std::error_code EC = windows::UTF8ToUTF16(Ext, U16Ext))
-      return EC;
-
     do {
       U16Result.reserve(Len);
       // Lets attach the extension manually. That is needed for files
       // with a point in name like aaa.bbb. SearchPathW will not add extension
       // from its argument to such files because it thinks they already had one.
       SmallVector<wchar_t, MAX_PATH> U16NameExt;
       if (std::error_code EC =
               windows::UTF8ToUTF16(Twine(Name + Ext).str(), U16NameExt))
         return EC;
 
       Len = ::SearchPathW(Path, c_str(U16NameExt), nullptr,
                           U16Result.capacity(), U16Result.data(), nullptr);
     } while (Len > U16Result.capacity());
 
-    if (Len != 0)
-      break; // Found it.
-  }
-
     if (Len == 0)
-    return mapWindowsError(::GetLastError());
+      continue;
 
     U16Result.set_size(Len);
 
-  SmallVector<char, MAX_PATH> U8Result;
     if (std::error_code EC =
-          windows::UTF16ToUTF8(U16Result.data(), U16Result.size(), U8Result))
       return EC;
 
+    if (sys::fs::can_execute(U8Result))
+      break; // Found it.
+
+    U8Result.clear();
+  }
+
+  if (U8Result.empty())
+    return mapWindowsError(::GetLastError());
+
   return std::string(U8Result.begin(), U8Result.end());
 }
 
 bool MakeErrMsg(std::string *ErrMsg, const std::string &prefix) {
   if (!ErrMsg)
     return true;
   char *buffer = NULL;
   DWORD LastError = GetLastError();

llvm/unittests/Support/Path.cpp

TEST_F(FileSystemTest, DirectoryIteration) {
   }
   EXPECT_EQ(I, E);
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel/a/b/c"));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel/a/b"));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel/a"));
   ASSERT_NO_ERROR(fs::remove(Twine(TestDirectory) + "/reclevel"));
 }
 
+TEST_F(FileSystemTest, DirectoryNotExecutable) {
+  ASSERT_EQ(fs::access(TestDirectory, sys::fs::AccessMode::Execute),
+            errc::permission_denied);
+}
+
 #ifdef LLVM_ON_UNIX
 TEST_F(FileSystemTest, BrokenSymlinkDirectoryIteration) {
   // Create a known hierarchy to recurse over.
   ASSERT_NO_ERROR(fs::create_directories(Twine(TestDirectory) + "/symlink"));
   ASSERT_NO_ERROR(
       fs::create_link("no_such_file", Twine(TestDirectory) + "/symlink/a"));
   ASSERT_NO_ERROR(
       fs::create_directories(Twine(TestDirectory) + "/symlink/b/bb"));