[NFC][lldb][windows] refactor the creation of inherited handles (llvm#170301)

Co-authored-by: Saleem Abdulrasool <[email protected]>

Author: 2 people

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,6 +14,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Program.h"
+#include "llvm/Support/WindowsError.h"
 
 #include <string>
 #include <vector>
@@ -86,10 +87,14 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info,
   error.Clear();
 
   std::string executable;
+  std::vector<HANDLE> inherited_handles;
   STARTUPINFOEXW startupinfoex = {};
   STARTUPINFOW &startupinfo = startupinfoex.StartupInfo;
   PROCESS_INFORMATION pi = {};
 
+  startupinfo.cb = sizeof(startupinfoex);
+  startupinfo.dwFlags |= STARTF_USESTDHANDLES;
+
   HANDLE stdin_handle = GetStdioHandle(launch_info, STDIN_FILENO);
   HANDLE stdout_handle = GetStdioHandle(launch_info, STDOUT_FILENO);
   HANDLE stderr_handle = GetStdioHandle(launch_info, STDERR_FILENO);
@@ -102,22 +107,13 @@ 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);
+  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();
+  }
+  inherited_handles = *inherited_handles_or_err;
 
   SIZE_T attributelist_size = 0;
   InitializeProcThreadAttributeList(/*lpAttributeList=*/nullptr,
@@ -136,22 +132,6 @@ 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();
-    }
-  }
 
   const char *hide_console_var =
       getenv("LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE");
@@ -215,6 +195,46 @@ 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;
+  STARTUPINFOW startupinfo = startupinfoex.StartupInfo;
+
+  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);
+
+  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);
+
+  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) {

llvm/include/llvm/IR/User.h

@@ -141,7 +141,8 @@ class User : public Value {
   LLVM_ABI void growHungoffUses(unsigned N, bool IsPhi = false);
 
 protected:
-  ~User() = default; // Use deleteValue() to delete a generic Instruction.
+  // Use deleteValue() to delete a generic User.
+  ~User();
 
 public:
   User(const User &) = delete;

llvm/lib/IR/User.cpp

@@ -135,6 +135,7 @@ bool User::isDroppable() const {
 
 void *User::allocateFixedOperandUser(size_t Size, unsigned Us,
                                      unsigned DescBytes) {
+  assert(Us > 0 && "Should have at least one operand");
   assert(Us < (1u << NumUserOperandsBits) && "Too many operands");
 
   static_assert(sizeof(DescriptorInfo) % sizeof(void *) == 0, "Required below");
@@ -189,31 +190,39 @@ void *User::operator new(size_t Size, HungOffOperandsAllocMarker) {
 //                         User operator delete Implementation
 //===----------------------------------------------------------------------===//
 
-// Repress memory sanitization, due to use-after-destroy by operator
-// delete. Bug report 24578 identifies this issue.
-LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void User::operator delete(void *Usr) {
+User::~User() {
   // Hung off uses use a single Use* before the User, while other subclasses
   // use a Use[] allocated prior to the user.
-  User *Obj = static_cast<User *>(Usr);
-  if (Obj->HasHungOffUses) {
-    assert(!Obj->HasDescriptor && "not supported!");
+  void *AllocStart = nullptr;
+  if (HasHungOffUses) {
+    assert(!HasDescriptor && "not supported!");
 
-    Use **HungOffOperandList = static_cast<Use **>(Usr) - 1;
+    Use **HungOffOperandList = reinterpret_cast<Use **>(this) - 1;
     // drop the hung off uses.
-    Use::zap(*HungOffOperandList, *HungOffOperandList + Obj->NumUserOperands,
+    Use::zap(*HungOffOperandList, *HungOffOperandList + NumUserOperands,
              /* Delete */ true);
-    ::operator delete(HungOffOperandList);
-  } else if (Obj->HasDescriptor) {
-    Use *UseBegin = static_cast<Use *>(Usr) - Obj->NumUserOperands;
-    Use::zap(UseBegin, UseBegin + Obj->NumUserOperands, /* Delete */ false);
+    AllocStart = HungOffOperandList;
+  } else if (HasDescriptor) {
+    Use *UseBegin = reinterpret_cast<Use *>(this) - NumUserOperands;
+    Use::zap(UseBegin, UseBegin + NumUserOperands, /* Delete */ false);
 
     auto *DI = reinterpret_cast<DescriptorInfo *>(UseBegin) - 1;
-    uint8_t *Storage = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
-    ::operator delete(Storage);
+    AllocStart = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
   } else {
-    Use *Storage = static_cast<Use *>(Usr) - Obj->NumUserOperands;
-    Use::zap(Storage, Storage + Obj->NumUserOperands,
+    Use *Storage = reinterpret_cast<Use *>(this) - NumUserOperands;
+    assert(NumUserOperands > 0 && "Should have at least one operand");
+    Use::zap(Storage, Storage + NumUserOperands,
              /* Delete */ false);
-    ::operator delete(Storage);
+    AllocStart = Storage;
   }
+
+  // Operator delete needs to know where the allocation started. To avoid
+  // use-after-destroy, we have to store the allocation start outside the User
+  // object memory. We always have at least one Use* before the User, so we can
+  // use that to store the allocation start.
+  ((void **)this)[-1] = AllocStart;
+}
+
+void User::operator delete(void *Usr) {
+  ::operator delete(((void **)Usr)[-1]);
 }