diff --git a/src/lib/platform/MSWindowsSession.cpp b/src/lib/platform/MSWindowsSession.cpp index 6a64e24b1..ad485eb2e 100644 --- a/src/lib/platform/MSWindowsSession.cpp +++ b/src/lib/platform/MSWindowsSession.cpp @@ -8,6 +8,7 @@ #include "arch/win32/XArchWindows.h" #include "base/Log.h" +#include "platform/MSWindowsHandle.h" #include #include @@ -19,8 +20,8 @@ MSWindowsSession::MSWindowsSession() : m_activeSessionId(-1) bool MSWindowsSession::isProcessInSession(const wchar_t *name, PHANDLE process = nullptr) { // first we need to take a snapshot of the running processes - HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); - if (snapshot == INVALID_HANDLE_VALUE) { + MSWindowsHandle snapshot(CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0)); + if (snapshot.get() == INVALID_HANDLE_VALUE) { LOG_ERR("could not get process snapshot"); throw std::runtime_error(windowsErrorToString(GetLastError())); } @@ -30,7 +31,7 @@ bool MSWindowsSession::isProcessInSession(const wchar_t *name, PHANDLE process = // get the first process, and if we can't do that then it's // unlikely we can go any further - BOOL gotEntry = Process32First(snapshot, &entry); + BOOL gotEntry = Process32First(snapshot.get(), &entry); if (!gotEntry) { LOG_ERR("could not get first process entry"); throw std::runtime_error(windowsErrorToString(GetLastError())); @@ -55,7 +56,7 @@ bool MSWindowsSession::isProcessInSession(const wchar_t *name, PHANDLE process = LOG_VERBOSE( "could not get session id for process: %i %s, code=%i", entry.th32ProcessID, entry.szExeFile, GetLastError() ); - gotEntry = nextProcessEntry(snapshot, &entry); + gotEntry = nextProcessEntry(snapshot.get(), &entry); continue; } else { // only pay attention to processes in the active session @@ -72,7 +73,7 @@ bool MSWindowsSession::isProcessInSession(const wchar_t *name, PHANDLE process = } // now move on to the next entry (if we're not at the end) - gotEntry = nextProcessEntry(snapshot, &entry); + gotEntry = nextProcessEntry(snapshot.get(), &entry); } std::wstring nameListJoin; @@ -83,13 +84,15 @@ bool MSWindowsSession::isProcessInSession(const wchar_t *name, PHANDLE process = LOG_VERBOSE("processes in session %d: %s", m_activeSessionId, nameListJoin.c_str()); - CloseHandle(snapshot); - if (pid) { if (process != nullptr) { // now get the process, which we'll use to get the process token. LOG_DEBUG("found %s in session %i", name, m_activeSessionId); *process = OpenProcess(MAXIMUM_ALLOWED, FALSE, pid); + if (*process == nullptr) { + LOG_WARN("found %s in session %i but could not open process handle", name, m_activeSessionId); + return false; + } } return true; } else { @@ -111,11 +114,12 @@ MSWindowsSession::getUserToken(LPSECURITY_ATTRIBUTES security) if (!DuplicateTokenEx( sourceToken, TOKEN_ASSIGN_PRIMARY | TOKEN_ALL_ACCESS, security, SecurityImpersonation, TokenPrimary, &newToken )) { - + CloseHandle(sourceToken); LOG_ERR("could not duplicate token"); throw std::runtime_error(windowsErrorToString(GetLastError())); } + CloseHandle(sourceToken); LOG_DEBUG("duplicated, new token: %i", newToken); return newToken; } diff --git a/src/lib/platform/MSWindowsWatchdog.cpp b/src/lib/platform/MSWindowsWatchdog.cpp index b7b9ad876..8f56b9a69 100644 --- a/src/lib/platform/MSWindowsWatchdog.cpp +++ b/src/lib/platform/MSWindowsWatchdog.cpp @@ -43,10 +43,14 @@ HANDLE openProcessForKill(const PROCESSENTRY32 &entry) return nullptr; } - HANDLE handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, entry.th32ProcessID); + const DWORD desiredAccess = PROCESS_TERMINATE | PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE; + HANDLE handle = OpenProcess(desiredAccess, FALSE, entry.th32ProcessID); if (handle == nullptr) { - LOG_ERR("could not open process handle for kill"); - throw std::runtime_error(windowsErrorToString(GetLastError())); + LOG_WARN( + "could not open process handle for kill, pid=%u, error=%s", entry.th32ProcessID, + windowsErrorToString(GetLastError()).c_str() + ); + return nullptr; } // only shut down if not current process (daemon is now the same unified binary). @@ -116,10 +120,12 @@ MSWindowsWatchdog::duplicateProcessToken(HANDLE process, LPSECURITY_ATTRIBUTES s ); if (!duplicateRet) { + CloseHandle(sourceToken); LOG_ERR("could not duplicate token %i", sourceToken); throw std::runtime_error(windowsErrorToString(GetLastError())); } + CloseHandle(sourceToken); LOG_DEBUG("duplicated, new token: %i", newToken); return newToken; } @@ -137,13 +143,22 @@ MSWindowsWatchdog::getUserToken(LPSECURITY_ATTRIBUTES security, bool elevatedTok if (!m_session.isProcessInSession(L"winlogon.exe", &process)) { throw std::runtime_error("cannot get user token without winlogon.exe"); } + if (process == nullptr) { + throw std::runtime_error("found winlogon.exe but failed to open process handle"); + } try { - return duplicateProcessToken(process, security); - } catch (std::runtime_error &e) { + HANDLE token = duplicateProcessToken(process, security); + if (process != nullptr) { + CloseHandle(process); + } + return token; + } catch (...) { LOG_ERR("failed to duplicate user token from winlogon.exe"); - CloseHandle(process); - throw e; + if (process != nullptr) { + CloseHandle(process); + } + throw; } } else { LOG_DEBUG("getting non-elevated token"); @@ -371,8 +386,8 @@ void MSWindowsWatchdog::shutdownExistingProcesses() const auto kAllProcesses = 0; // first we need to take a snapshot of the running processes - HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, kAllProcesses); - if (snapshot == INVALID_HANDLE_VALUE) { + MSWindowsHandle snapshot(CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, kAllProcesses)); + if (snapshot.get() == INVALID_HANDLE_VALUE) { LOG_ERR("could not get process snapshot"); throw std::runtime_error(windowsErrorToString(GetLastError())); } @@ -382,7 +397,7 @@ void MSWindowsWatchdog::shutdownExistingProcesses() // get the first process, and if we can't do that then it's // unlikely we can go any further - BOOL gotEntry = Process32First(snapshot, &entry); + BOOL gotEntry = Process32First(snapshot.get(), &entry); if (!gotEntry) { LOG_ERR("could not get first process entry"); throw std::runtime_error(windowsErrorToString(GetLastError())); @@ -398,7 +413,7 @@ void MSWindowsWatchdog::shutdownExistingProcesses() } // now move on to the next entry (if we're not at the end) - gotEntry = Process32Next(snapshot, &entry); + gotEntry = Process32Next(snapshot.get(), &entry); if (!gotEntry) { DWORD err = GetLastError(); @@ -410,8 +425,6 @@ void MSWindowsWatchdog::shutdownExistingProcesses() } } } - - CloseHandle(snapshot); } MSWindowsWatchdog::ProcessState MSWindowsWatchdog::handleStartError(const std::string_view &message)