diff --git a/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h b/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h index 81aea5b2022a5..553263e2f5a72 100644 --- a/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h +++ b/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` containing all handles that the child must + /// inherit. + llvm::ErrorOr> + GetInheritedHandles(const ProcessLaunchInfo &launch_info, + STARTUPINFOEXW &startupinfoex, + HANDLE stdout_handle = NULL, HANDLE stderr_handle = NULL, + HANDLE stdin_handle = NULL); }; } diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp index f5adadaf061bf..ac5383f20f58e 100644 --- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp +++ b/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 #include @@ -21,42 +22,63 @@ using namespace lldb; using namespace lldb_private; -static void CreateEnvironmentBuffer(const Environment &env, - std::vector &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 CreateEnvironmentBufferW(const Environment &env) { + std::vector env_entries; for (const auto &KV : env) { - std::wstring warg; - if (llvm::ConvertUTF8toWide(Environment::compose(KV), warg)) { - buffer.insert( - buffer.end(), reinterpret_cast(warg.c_str()), - reinterpret_cast(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 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 +GetFlattenedWindowsCommandStringW(Args args) { if (args.empty()) - return false; + return L""; std::vector args_ref; for (auto &entry : args.entries()) args_ref.push_back(entry.ref()); - llvm::ErrorOr result = - llvm::sys::flattenWindowsCommandLine(args_ref); - if (result.getError()) - return false; - - command = *result; - return true; + return llvm::sys::flattenWindowsCommandLine(args_ref); } HostProcess @@ -64,11 +86,9 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, Status &error) { error.Clear(); - std::string executable; - std::vector 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 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(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 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 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(&startupinfoex), &pi); + reinterpret_cast(&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> ProcessLauncherWindows::GetInheritedHandles( + const ProcessLaunchInfo &launch_info, STARTUPINFOEXW &startupinfoex, + HANDLE stdout_handle, HANDLE stderr_handle, HANDLE stdin_handle) { + std::vector 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(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) { diff --git a/lldb/source/Interpreter/ScriptInterpreter.cpp b/lldb/source/Interpreter/ScriptInterpreter.cpp index cee5f94b8aa29..e62ce1d68472d 100644 --- a/lldb/source/Interpreter/ScriptInterpreter.cpp +++ b/lldb/source/Interpreter/ScriptInterpreter.cpp @@ -143,7 +143,7 @@ SymbolContext ScriptInterpreter::GetOpaqueTypeFromSBSymbolContext( return {}; } -std::optional +std::optional ScriptInterpreter::GetOpaqueTypeFromSBMemoryRegionInfo( const lldb::SBMemoryRegionInfo &mem_region) const { if (!mem_region.m_opaque_up) diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp index 48fc58020e2ca..aa19709189875 100644 --- a/lldb/source/Target/Process.cpp +++ b/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 ®ion) { +CreateCoreFileMemoryRange(const lldb_private::MemoryRegionInfo ®ion) { 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 ®ion) { // 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 ®ion, +static bool AddDirtyPages(const lldb_private::MemoryRegionInfo ®ion, CoreFileMemoryRanges &ranges) { const auto &dirty_page_list = region.GetDirtyPageList(); if (!dirty_page_list) @@ -6827,8 +6827,8 @@ static bool AddDirtyPages(const MemoryRegionInfo ®ion, // 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 ®ion, bool try_dirty_pages, - CoreFileMemoryRanges &ranges) { +static void AddRegion(const lldb_private::MemoryRegionInfo ®ion, + 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 dynamic_loader_mem_regions; + std::vector dynamic_loader_mem_regions; std::function 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); diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp index 012eae02d5857..966b37e09ee55 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp @@ -326,7 +326,7 @@ TEST_F(GDBRemoteCommunicationClientTest, SendSignalsToIgnore) { TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfo) { const lldb::addr_t addr = 0xa000; - MemoryRegionInfo region_info; + lldb_private::MemoryRegionInfo region_info; std::future result = std::async(std::launch::async, [&] { return client.GetMemoryRegionInfo(addr, region_info); }); @@ -343,13 +343,16 @@ TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfo) { EXPECT_TRUE(result.get().Success()); EXPECT_EQ(addr, region_info.GetRange().GetRangeBase()); EXPECT_EQ(0x2000u, region_info.GetRange().GetByteSize()); - EXPECT_EQ(MemoryRegionInfo::eYes, region_info.GetReadable()); - EXPECT_EQ(MemoryRegionInfo::eNo, region_info.GetWritable()); - EXPECT_EQ(MemoryRegionInfo::eYes, region_info.GetExecutable()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eYes, region_info.GetReadable()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eNo, region_info.GetWritable()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eYes, region_info.GetExecutable()); EXPECT_EQ("/foo/bar.so", region_info.GetName().GetStringRef()); - EXPECT_EQ(MemoryRegionInfo::eDontKnow, region_info.GetMemoryTagged()); - EXPECT_EQ(MemoryRegionInfo::eDontKnow, region_info.IsStackMemory()); - EXPECT_EQ(MemoryRegionInfo::eDontKnow, region_info.IsShadowStack()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eDontKnow, + region_info.GetMemoryTagged()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eDontKnow, + region_info.IsStackMemory()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eDontKnow, + region_info.IsShadowStack()); result = std::async(std::launch::async, [&] { return client.GetMemoryRegionInfo(addr, region_info); @@ -358,9 +361,9 @@ TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfo) { HandlePacket(server, "qMemoryRegionInfo:a000", "start:a000;size:2000;flags:;type:stack;"); EXPECT_TRUE(result.get().Success()); - EXPECT_EQ(MemoryRegionInfo::eNo, region_info.GetMemoryTagged()); - EXPECT_EQ(MemoryRegionInfo::eYes, region_info.IsStackMemory()); - EXPECT_EQ(MemoryRegionInfo::eNo, region_info.IsShadowStack()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eNo, region_info.GetMemoryTagged()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eYes, region_info.IsStackMemory()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eNo, region_info.IsShadowStack()); result = std::async(std::launch::async, [&] { return client.GetMemoryRegionInfo(addr, region_info); @@ -369,9 +372,10 @@ TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfo) { HandlePacket(server, "qMemoryRegionInfo:a000", "start:a000;size:2000;flags: mt zz mt ss ;type:ha,ha,stack;"); EXPECT_TRUE(result.get().Success()); - EXPECT_EQ(MemoryRegionInfo::eYes, region_info.GetMemoryTagged()); - EXPECT_EQ(MemoryRegionInfo::eYes, region_info.IsStackMemory()); - EXPECT_EQ(MemoryRegionInfo::eYes, region_info.IsShadowStack()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eYes, + region_info.GetMemoryTagged()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eYes, region_info.IsStackMemory()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eYes, region_info.IsShadowStack()); result = std::async(std::launch::async, [&] { return client.GetMemoryRegionInfo(addr, region_info); @@ -380,12 +384,12 @@ TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfo) { HandlePacket(server, "qMemoryRegionInfo:a000", "start:a000;size:2000;type:heap;"); EXPECT_TRUE(result.get().Success()); - EXPECT_EQ(MemoryRegionInfo::eNo, region_info.IsStackMemory()); + EXPECT_EQ(lldb_private::MemoryRegionInfo::eNo, region_info.IsStackMemory()); } TEST_F(GDBRemoteCommunicationClientTest, GetMemoryRegionInfoInvalidResponse) { const lldb::addr_t addr = 0x4000; - MemoryRegionInfo region_info; + lldb_private::MemoryRegionInfo region_info; std::future result = std::async(std::launch::async, [&] { return client.GetMemoryRegionInfo(addr, region_info); });