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

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.

Differential Revision: https://reviews.llvm.org/D99357

diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index adcbd1b5f8f31a1d737dbacfd2f86eed3a40d1a9..e2bd7da4f04bf68d9e2bee458ce99ce22c0920e4 100644 (file)
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -623,6 +623,9 @@ std::error_code access(const Twine &Path, AccessMode Mode) {
   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();
 }
 

diff --git a/llvm/lib/Support/Windows/Program.inc b/llvm/lib/Support/Windows/Program.inc
index f1d612cf3c98295f301dacacaada6690631375ab..687109c01355e758e05bc7d8be2efe3d36833737 100644 (file)
--- a/llvm/lib/Support/Windows/Program.inc
+++ b/llvm/lib/Support/Windows/Program.inc
@@ -67,13 +67,10 @@ ErrorOr<std::string> sys::findProgramByName(StringRef Name,
   if (const char *PathExtEnv = std::getenv("PATHEXT"))
     SplitString(PathExtEnv, PathExts, ";");
 
-  SmallVector<wchar_t, MAX_PATH> U16Result;
-  DWORD Len = MAX_PATH;
+  SmallVector<char, MAX_PATH> U8Result;
   for (StringRef Ext : PathExts) {
-    SmallVector<wchar_t, MAX_PATH> U16Ext;
-    if (std::error_code EC = windows::UTF8ToUTF16(Ext, U16Ext))
-      return EC;
-
+    SmallVector<wchar_t, MAX_PATH> U16Result;
+    DWORD Len = MAX_PATH;
     do {
       U16Result.reserve(Len);
       // Lets attach the extension manually. That is needed for files
@@ -88,20 +85,24 @@ ErrorOr<std::string> sys::findProgramByName(StringRef Name,
                           U16Result.capacity(), U16Result.data(), nullptr);
     } while (Len > U16Result.capacity());
 
-    if (Len != 0)
+    if (Len == 0)
+      continue;
+
+    U16Result.set_size(Len);
+
+    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 (Len == 0)
+  if (U8Result.empty())
     return mapWindowsError(::GetLastError());
 
-  U16Result.set_size(Len);
-
-  SmallVector<char, MAX_PATH> U8Result;
-  if (std::error_code EC =
-          windows::UTF16ToUTF8(U16Result.data(), U16Result.size(), U8Result))
-    return EC;
-
   return std::string(U8Result.begin(), U8Result.end());
 }
 

diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index ef1877381b0492bdc574f9c80faf31cb46e835d7..73f0cbbaae38eb5d7eec52fe70f18d3aaa32433a 100644 (file)
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -1088,6 +1088,11 @@ TEST_F(FileSystemTest, DirectoryIteration) {
   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.