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

[Markus Böck via Phabricator via llvm-commits]

zero9178 created this revision.
zero9178 added reviewers: rnk, Bigcheese.
Herald added subscribers: dexonsmith, hiraditya.
zero9178 requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99357

Files:
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/unittests/Support/Path.cpp


Index: llvm/unittests/Support/Path.cpp
===================================================================
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1088,6 +1088,11 @@
   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.
Index: llvm/lib/Support/Windows/Program.inc
===================================================================
--- llvm/lib/Support/Windows/Program.inc
+++ llvm/lib/Support/Windows/Program.inc
@@ -67,13 +67,10 @@
   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 @@
                           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());
 }
 
Index: llvm/lib/Support/Windows/Path.inc
===================================================================
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -623,6 +623,9 @@
   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();
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D99357.333343.patch
Type: text/x-patch
Size: 2625 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210325/d50685dc/attachment.bin>