🍒 [lldb][windows] fix environment handling in CreateProcessW setup

lldb/include/lldb/Host/windows/ProcessLauncherWindows.h

@@ -11,6 +11,7 @@
 
 #include "lldb/Host/ProcessLauncher.h"
 #include "lldb/Host/windows/windows.h"
+#include "llvm/Support/ErrorOr.h"
 
 namespace lldb_private {
 
@@ -23,6 +24,36 @@ class ProcessLauncherWindows : public ProcessLauncher {
 
 protected:
   HANDLE GetStdioHandle(const ProcessLaunchInfo &launch_info, int fd);
+
+  /// Get the list of Windows handles that should be inherited by the child
+  /// process and update `STARTUPINFOEXW` with the handle list.
+  ///
+  /// If no handles need to be inherited, an empty vector is returned.
+  ///
+  /// Otherwise, the function populates the
+  /// `PROC_THREAD_ATTRIBUTE_HANDLE_LIST` attribute in `startupinfoex` with the
+  /// collected handles using `UpdateProcThreadAttribute`. On success, the
+  /// vector of inherited handles is returned.
+  ///
+  /// \param launch_info
+  ///   The process launch configuration.
+  ///
+  /// \param startupinfoex
+  ///   The extended STARTUPINFO structure for the process being created.
+  ///
+  /// \param stdout_handle
+  /// \param stderr_handle
+  /// \param stdin_handle
+  ///   Optional explicit standard stream handles to use for the child process.
+  ///
+  /// \returns
+  ///   `std::vector<HANDLE>` containing all handles that the child must
+  ///   inherit.
+  llvm::ErrorOr<std::vector<HANDLE>>
+  GetInheritedHandles(const ProcessLaunchInfo &launch_info,
+                      STARTUPINFOEXW &startupinfoex,
+                      HANDLE stdout_handle = NULL, HANDLE stderr_handle = NULL,
+                      HANDLE stdin_handle = NULL);
 };
 }
 

lldb/source/Host/windows/ProcessLauncherWindows.cpp

@@ -14,61 +14,81 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Program.h"
+#include "llvm/Support/WindowsError.h"
 
 #include <string>
 #include <vector>
 
 using namespace lldb;
 using namespace lldb_private;
 
-static void CreateEnvironmentBuffer(const Environment &env,
-                                    std::vector<char> &buffer) {
-  // The buffer is a list of null-terminated UTF-16 strings, followed by an
-  // extra L'\0' (two bytes of 0).  An empty environment must have one
-  // empty string, followed by an extra L'\0'.
+/// Create a UTF-16 environment block to use with CreateProcessW.
+///
+/// The buffer is a sequence of null-terminated UTF-16 strings, followed by an
+/// extra L'\0' (two bytes of 0). An empty environment must have one
+/// empty string, followed by an extra L'\0'.
+///
+/// The keys are sorted to comply with the CreateProcess API calling convention.
+///
+/// Ensure that the resulting buffer is used in conjunction with
+/// CreateProcessW and be sure that dwCreationFlags includes
+/// CREATE_UNICODE_ENVIRONMENT.
+///
+/// \param env The Environment object to convert.
+/// \returns The sorted sequence of environment variables and their values,
+/// separated by null terminators. The vector is guaranteed to never be empty.
+static std::vector<wchar_t> CreateEnvironmentBufferW(const Environment &env) {
+  std::vector<std::wstring> env_entries;
   for (const auto &KV : env) {
-    std::wstring warg;
-    if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) {
-      buffer.insert(
-          buffer.end(), reinterpret_cast<const char *>(warg.c_str()),
-          reinterpret_cast<const char *>(warg.c_str() + warg.size() + 1));
-    }
+    std::wstring wentry;
+    if (llvm::ConvertUTF8toWide(Environment::compose(KV), wentry))
+      env_entries.push_back(std::move(wentry));
   }
-  // One null wchar_t (to end the block) is two null bytes
-  buffer.push_back(0);
-  buffer.push_back(0);
-  // Insert extra two bytes, just in case the environment was empty.
-  buffer.push_back(0);
-  buffer.push_back(0);
+  std::sort(env_entries.begin(), env_entries.end(),
+            [](const std::wstring &a, const std::wstring &b) {
+              return _wcsicmp(a.c_str(), b.c_str()) < 0;
+            });
+
+  std::vector<wchar_t> buffer;
+  for (const auto &env_entry : env_entries) {
+    buffer.insert(buffer.end(), env_entry.begin(), env_entry.end());
+    buffer.push_back(L'\0');
+  }
+
+  if (buffer.empty())
+    buffer.push_back(L'\0'); // If there are no environment variables, we have
+                             // to ensure there are 4 zero bytes in the buffer.
+  buffer.push_back(L'\0');
+
+  return buffer;
 }
 
-static bool GetFlattenedWindowsCommandString(Args args, std::wstring &command) {
+/// Flattens an Args object into a Windows command-line wide string.
+///
+/// Returns an empty string if args is empty.
+///
+/// \param args The Args object to flatten.
+/// \returns A wide string containing the flattened command line.
+static llvm::ErrorOr<std::wstring>
+GetFlattenedWindowsCommandStringW(Args args) {
   if (args.empty())
-    return false;
+    return L"";
 
   std::vector<llvm::StringRef> args_ref;
   for (auto &entry : args.entries())
     args_ref.push_back(entry.ref());
 
-  llvm::ErrorOr<std::wstring> result =
-      llvm::sys::flattenWindowsCommandLine(args_ref);
-  if (result.getError())
-    return false;
-
-  command = *result;
-  return true;
+  return llvm::sys::flattenWindowsCommandLine(args_ref);
 }
 
 HostProcess
 ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
                                       Status &error) {
   error.Clear();
 
-  std::string executable;
-  std::vector<char> environment;
-  STARTUPINFOEX startupinfoex = {};
-  STARTUPINFO &startupinfo = startupinfoex.StartupInfo;
-  PROCESS_INFORMATION pi = {};
+  STARTUPINFOEXW startupinfoex = {};
+  startupinfoex.StartupInfo.cb = sizeof(startupinfoex);
+  startupinfoex.StartupInfo.dwFlags |= STARTF_USESTDHANDLES;
 
   HANDLE stdin_handle = GetStdioHandle(launch_info, STDIN_FILENO);
   HANDLE stdout_handle = GetStdioHandle(launch_info, STDOUT_FILENO);
@@ -82,23 +102,6 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
       ::CloseHandle(stderr_handle);
   });
 
-  startupinfo.cb = sizeof(startupinfoex);
-  startupinfo.dwFlags |= STARTF_USESTDHANDLES;
-  startupinfo.hStdError =
-      stderr_handle ? stderr_handle : ::GetStdHandle(STD_ERROR_HANDLE);
-  startupinfo.hStdInput =
-      stdin_handle ? stdin_handle : ::GetStdHandle(STD_INPUT_HANDLE);
-  startupinfo.hStdOutput =
-      stdout_handle ? stdout_handle : ::GetStdHandle(STD_OUTPUT_HANDLE);
-
-  std::vector<HANDLE> inherited_handles;
-  if (startupinfo.hStdError)
-    inherited_handles.push_back(startupinfo.hStdError);
-  if (startupinfo.hStdInput)
-    inherited_handles.push_back(startupinfo.hStdInput);
-  if (startupinfo.hStdOutput)
-    inherited_handles.push_back(startupinfo.hStdOutput);
-
   SIZE_T attributelist_size = 0;
   InitializeProcThreadAttributeList(/*lpAttributeList=*/nullptr,
                                     /*dwAttributeCount=*/1, /*dwFlags=*/0,
@@ -116,29 +119,21 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
   }
   auto delete_attributelist = llvm::make_scope_exit(
       [&] { DeleteProcThreadAttributeList(startupinfoex.lpAttributeList); });
-  for (size_t i = 0; i < launch_info.GetNumFileActions(); ++i) {
-    const FileAction *act = launch_info.GetFileActionAtIndex(i);
-    if (act->GetAction() == FileAction::eFileActionDuplicate &&
-        act->GetFD() == act->GetActionArgument())
-      inherited_handles.push_back(reinterpret_cast<HANDLE>(act->GetFD()));
-  }
-  if (!inherited_handles.empty()) {
-    if (!UpdateProcThreadAttribute(
-            startupinfoex.lpAttributeList, /*dwFlags=*/0,
-            PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inherited_handles.data(),
-            inherited_handles.size() * sizeof(HANDLE),
-            /*lpPreviousValue=*/nullptr, /*lpReturnSize=*/nullptr)) {
-      error = Status(::GetLastError(), eErrorTypeWin32);
-      return HostProcess();
-    }
+
+  auto inherited_handles_or_err = GetInheritedHandles(
+      launch_info, startupinfoex, stdout_handle, stderr_handle, stdin_handle);
+  if (!inherited_handles_or_err) {
+    error = Status(inherited_handles_or_err.getError());
+    return HostProcess();
   }
+  std::vector<HANDLE> inherited_handles = *inherited_handles_or_err;
 
   const char *hide_console_var =
       getenv("LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE");
   if (hide_console_var &&
       llvm::StringRef(hide_console_var).equals_insensitive("true")) {
-    startupinfo.dwFlags |= STARTF_USESHOWWINDOW;
-    startupinfo.wShowWindow = SW_HIDE;
+    startupinfoex.StartupInfo.dwFlags |= STARTF_USESHOWWINDOW;
+    startupinfoex.StartupInfo.wShowWindow = SW_HIDE;
   }
 
   DWORD flags = CREATE_NEW_CONSOLE | CREATE_UNICODE_ENVIRONMENT |
@@ -149,28 +144,34 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
   if (launch_info.GetFlags().Test(eLaunchFlagDisableSTDIO))
     flags &= ~CREATE_NEW_CONSOLE;
 
-  LPVOID env_block = nullptr;
-  ::CreateEnvironmentBuffer(launch_info.GetEnvironment(), environment);
-  env_block = environment.data();
+  std::vector<wchar_t> environment =
+      CreateEnvironmentBufferW(launch_info.GetEnvironment());
 
-  executable = launch_info.GetExecutableFile().GetPath();
-  std::wstring wcommandLine;
-  GetFlattenedWindowsCommandString(launch_info.GetArguments(), wcommandLine);
-
-  std::wstring wexecutable, wworkingDirectory;
-  llvm::ConvertUTF8toWide(executable, wexecutable);
-  llvm::ConvertUTF8toWide(launch_info.GetWorkingDirectory().GetPath(),
-                          wworkingDirectory);
+  auto wcommandLineOrErr =
+      GetFlattenedWindowsCommandStringW(launch_info.GetArguments());
+  if (!wcommandLineOrErr) {
+    error = Status(wcommandLineOrErr.getError());
+    return HostProcess();
+  }
+  std::wstring wcommandLine = *wcommandLineOrErr;
   // If the command line is empty, it's best to pass a null pointer to tell
   // CreateProcessW to use the executable name as the command line.  If the
   // command line is not empty, its contents may be modified by CreateProcessW.
   WCHAR *pwcommandLine = wcommandLine.empty() ? nullptr : &wcommandLine[0];
 
+  std::wstring wexecutable, wworkingDirectory;
+  llvm::ConvertUTF8toWide(launch_info.GetExecutableFile().GetPath(),
+                          wexecutable);
+  llvm::ConvertUTF8toWide(launch_info.GetWorkingDirectory().GetPath(),
+                          wworkingDirectory);
+
+  PROCESS_INFORMATION pi = {};
+
   BOOL result = ::CreateProcessW(
       wexecutable.c_str(), pwcommandLine, NULL, NULL,
-      /*bInheritHandles=*/!inherited_handles.empty(), flags, env_block,
+      /*bInheritHandles=*/!inherited_handles.empty(), flags, environment.data(),
       wworkingDirectory.size() == 0 ? NULL : wworkingDirectory.c_str(),
-      reinterpret_cast<STARTUPINFO *>(&startupinfoex), &pi);
+      reinterpret_cast<STARTUPINFOW *>(&startupinfoex), &pi);
 
   if (!result) {
     // Call GetLastError before we make any other system calls.
@@ -191,6 +192,45 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
   return HostProcess(pi.hProcess);
 }
 
+llvm::ErrorOr<std::vector<HANDLE>> ProcessLauncherWindows::GetInheritedHandles(
+    const ProcessLaunchInfo &launch_info, STARTUPINFOEXW &startupinfoex,
+    HANDLE stdout_handle, HANDLE stderr_handle, HANDLE stdin_handle) {
+  std::vector<HANDLE> inherited_handles;
+
+  startupinfoex.StartupInfo.hStdError =
+      stderr_handle ? stderr_handle : GetStdHandle(STD_ERROR_HANDLE);
+  startupinfoex.StartupInfo.hStdInput =
+      stdin_handle ? stdin_handle : GetStdHandle(STD_INPUT_HANDLE);
+  startupinfoex.StartupInfo.hStdOutput =
+      stdout_handle ? stdout_handle : GetStdHandle(STD_OUTPUT_HANDLE);
+
+  if (startupinfoex.StartupInfo.hStdError)
+    inherited_handles.push_back(startupinfoex.StartupInfo.hStdError);
+  if (startupinfoex.StartupInfo.hStdInput)
+    inherited_handles.push_back(startupinfoex.StartupInfo.hStdInput);
+  if (startupinfoex.StartupInfo.hStdOutput)
+    inherited_handles.push_back(startupinfoex.StartupInfo.hStdOutput);
+
+  for (size_t i = 0; i < launch_info.GetNumFileActions(); ++i) {
+    const FileAction *act = launch_info.GetFileActionAtIndex(i);
+    if (act->GetAction() == FileAction::eFileActionDuplicate &&
+        act->GetFD() == act->GetActionArgument())
+      inherited_handles.push_back(reinterpret_cast<HANDLE>(act->GetFD()));
+  }
+
+  if (inherited_handles.empty())
+    return inherited_handles;
+
+  if (!UpdateProcThreadAttribute(
+          startupinfoex.lpAttributeList, /*dwFlags=*/0,
+          PROC_THREAD_ATTRIBUTE_HANDLE_LIST, inherited_handles.data(),
+          inherited_handles.size() * sizeof(HANDLE),
+          /*lpPreviousValue=*/nullptr, /*lpReturnSize=*/nullptr))
+    return llvm::mapWindowsError(::GetLastError());
+
+  return inherited_handles;
+}
+
 HANDLE
 ProcessLauncherWindows::GetStdioHandle(const ProcessLaunchInfo &launch_info,
                                        int fd) {

lldb/source/Interpreter/ScriptInterpreter.cpp

@@ -143,7 +143,7 @@ SymbolContext ScriptInterpreter::GetOpaqueTypeFromSBSymbolContext(
   return {};
 }
 
-std::optional<MemoryRegionInfo>
+std::optional<lldb_private::MemoryRegionInfo>
 ScriptInterpreter::GetOpaqueTypeFromSBMemoryRegionInfo(
     const lldb::SBMemoryRegionInfo &mem_region) const {
   if (!mem_region.m_opaque_up)

lldb/source/Target/Process.cpp

@@ -6779,7 +6779,7 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, size_t len,
 
 // Create a CoreFileMemoryRange from a MemoryRegionInfo
 static CoreFileMemoryRange
-CreateCoreFileMemoryRange(const MemoryRegionInfo &region) {
+CreateCoreFileMemoryRange(const lldb_private::MemoryRegionInfo &region) {
   const addr_t addr = region.GetRange().GetRangeBase();
   llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
   return {range, region.GetLLDBPermissions()};
@@ -6788,7 +6788,7 @@ CreateCoreFileMemoryRange(const MemoryRegionInfo &region) {
 // Add dirty pages to the core file ranges and return true if dirty pages
 // were added. Return false if the dirty page information is not valid or in
 // the region.
-static bool AddDirtyPages(const MemoryRegionInfo &region,
+static bool AddDirtyPages(const lldb_private::MemoryRegionInfo &region,
                           CoreFileMemoryRanges &ranges) {
   const auto &dirty_page_list = region.GetDirtyPageList();
   if (!dirty_page_list)
@@ -6827,8 +6827,8 @@ static bool AddDirtyPages(const MemoryRegionInfo &region,
 // given region. If the region has dirty page information, only dirty pages
 // will be added to \a ranges, else the entire range will be added to \a
 // ranges.
-static void AddRegion(const MemoryRegionInfo &region, bool try_dirty_pages,
-                      CoreFileMemoryRanges &ranges) {
+static void AddRegion(const lldb_private::MemoryRegionInfo &region,
+                      bool try_dirty_pages, CoreFileMemoryRanges &ranges) {
   // Don't add empty ranges.
   if (region.GetRange().GetByteSize() == 0)
     return;
@@ -6851,7 +6851,7 @@ static void SaveDynamicLoaderSections(Process &process,
   if (!dyld)
     return;
 
-  std::vector<MemoryRegionInfo> dynamic_loader_mem_regions;
+  std::vector<lldb_private::MemoryRegionInfo> dynamic_loader_mem_regions;
   std::function<bool(const lldb_private::Thread &)> save_thread_predicate =
       [&](const lldb_private::Thread &t) -> bool {
     return options.ShouldThreadBeSaved(t.GetID());
@@ -6976,10 +6976,11 @@ static void GetCoreFileSaveRangesStackOnly(Process &process,
 
 // TODO: We should refactor CoreFileMemoryRanges to use the lldb range type, and
 // then add an intersect method on it, or MemoryRegionInfo.
-static MemoryRegionInfo Intersect(const MemoryRegionInfo &lhs,
-                                  const MemoryRegionInfo::RangeType &rhs) {
+static lldb_private::MemoryRegionInfo
+Intersect(const lldb_private::MemoryRegionInfo &lhs,
+          const lldb_private::MemoryRegionInfo::RangeType &rhs) {
 
-  MemoryRegionInfo region_info;
+  lldb_private::MemoryRegionInfo region_info;
   region_info.SetLLDBPermissions(lhs.GetLLDBPermissions());
   region_info.GetRange() = lhs.GetRange().Intersect(rhs);