From 07f2c432d9453a856c92f1d60aecf71d51d1fe64 Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 12:07:55 +0300 Subject: [PATCH 01/13] Added assert that remote device is not nullptr. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index c6d4e910ca..393724cab9 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -868,6 +868,7 @@ PTF_TEST_CASE(TestRemoteCapture) PTF_ASSERT_EQUAL(remoteDevices->getRemoteMachinePort(), remoteDevicePort); pcpp::PcapRemoteDevice* remoteDevice = remoteDevices->getRemoteDeviceByIP(remoteDeviceIPAddr); + PTF_ASSERT_NOT_NULL(remoteDevice); PTF_ASSERT_EQUAL(remoteDevice->getDeviceType(), pcpp::PcapLiveDevice::RemoteDevice, enum); PTF_ASSERT_EQUAL(remoteDevice->getMtu(), 0); pcpp::Logger::getInstance().suppressLogs(); From bcfad1fb8856c27b3d4f6beafc349db158f1b8ed Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 13:16:07 +0300 Subject: [PATCH 02/13] Refactored Rpcapd server object to be assigned to a win32 job object so it is guaranteed to be killed. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 91 +++++++++++++++++----- 1 file changed, 70 insertions(+), 21 deletions(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 393724cab9..92a3080828 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -97,17 +97,64 @@ class RpcapdServerInitializer { private: HANDLE m_ProcessHandle; + HANDLE m_JobHandle; -public: + void killProcessAndCloseHandles() + { + if (m_ProcessHandle != nullptr) + { + TerminateProcess(m_ProcessHandle, 0); + CloseHandle(m_ProcessHandle); + m_ProcessHandle = nullptr; + } - RpcapdServerInitializer(bool activateRemoteDevice, const std::string &ip, uint16_t port) : m_ProcessHandle(nullptr) + if (m_JobHandle != nullptr) + { + CloseHandle(m_JobHandle); + m_JobHandle = nullptr; + } + } +public: + RpcapdServerInitializer(bool activateRemoteDevice, const std::string& ip, uint16_t port) + : m_ProcessHandle(nullptr), m_JobHandle(nullptr) { if (!activateRemoteDevice) return; std::string cmd = "rpcapd\\rpcapd.exe"; - std::ostringstream args; - args << "rpcapd\\rpcapd.exe -b " << ip << " -p " << port << " -n"; + std::string args; + // Reserves the size of the arguments string in 1 allocation. + // Sizeof C-string includes the NULL terminator in the size. + // For the purposes of this calculation the NULL terminator's inclusion in the size is used to simulate the space delimiter after the argument. + args.reserve( + sizeof("-b") + ip.size() + + sizeof(" -p") + 5 /* The maximum digits a uint16_t can have is 5 */ + + sizeof(" -n") - 1 /* Subtracts one as the last NULL terminator is not needed */ + ); + args += "-b "; + args += ip; + args += " -p"; + args += std::to_string(port); + args += " -n"; + + m_JobHandle = CreateJobObject(nullptr, nullptr); + if (m_JobHandle == nullptr) + { + throw std::runtime_error("Failed to create a job object with error code: " + std::to_string(GetLastError())); + } + + // Sets up the job limits so closing the job will automatically kill all processes assigned to the job. + // This will prevent the subprocess continuing to live if the current process is killed without unwinding the stack (i.e. std::terminate is called), + // as the OS itself will kill the subprocesses when the last job handle is closed. + JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimitInfo; + ZeroMemory(&jobLimitInfo, sizeof(jobLimitInfo)); + jobLimitInfo.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + if (!SetInformationJobObject(m_JobHandle, JobObjectExtendedLimitInformation ,&jobLimitInfo, sizeof(jobLimitInfo))) + { + DWORD errCode = GetLastError(); + CloseHandle(m_JobHandle); + throw std::runtime_error("Failed to set settings to job object with error code: " + std::to_string(errCode)); + } STARTUPINFO si; PROCESS_INFORMATION pi; @@ -115,36 +162,38 @@ class RpcapdServerInitializer ZeroMemory( &si, sizeof(si) ); si.cb = sizeof(si); ZeroMemory( &pi, sizeof(pi) ); - if (!CreateProcess - ( + if (!CreateProcess ( TEXT(cmd.c_str()), - const_cast(TEXT(args.str().c_str())), // TODO: This can potentially cause access violation if Unicode version CreateProcessW is chosen. - NULL,NULL,FALSE, + const_cast(TEXT(args.c_str())), // TODO: This can potentially cause access violation if Unicode version CreateProcessW is chosen. + nullptr, nullptr, false, CREATE_NEW_CONSOLE, - NULL,NULL, + nullptr, nullptr, &si, &pi - ) - ) + )) { - m_ProcessHandle = NULL; - PCPP_LOG_ERROR("Create process failed " << static_cast(GetLastError())); - return; + DWORD errCode = GetLastError(); + CloseHandle(m_JobHandle); + throw std::runtime_error("Create process failed with error code: " + std::to_string(errCode)); } m_ProcessHandle = pi.hProcess; - } + CloseHandle(pi.hThread); // We don't need the thread handle, so we can close it. - ~RpcapdServerInitializer() + if (!AssignProcessToJobObject(m_JobHandle, m_ProcessHandle)) { - if (m_ProcessHandle != NULL) - { - TerminateProcess(m_ProcessHandle, 0); - CloseHandle(m_ProcessHandle); + DWORD errCode = GetLastError(); + killProcessAndCloseHandles(); + throw std::runtime_error("Failed assigning process to job object with code: " + std::to_string(errCode)); } } - HANDLE getHandle() { return m_ProcessHandle; } + RpcapdServerInitializer(const RpcapdServerInitializer&) = delete; + RpcapdServerInitializer& operator=(const RpcapdServerInitializer&) = delete; + + ~RpcapdServerInitializer() { killProcessAndCloseHandles(); } + + HANDLE getHandle() const { return m_ProcessHandle; } }; #endif // defined(_WIN32) From 21c79f43b3b432de6ec4d1e8beaac9d0a5e8caba Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 13:21:52 +0300 Subject: [PATCH 03/13] Fixed arguments. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 92a3080828..33351be41d 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -109,7 +109,7 @@ class RpcapdServerInitializer } if (m_JobHandle != nullptr) - { + { CloseHandle(m_JobHandle); m_JobHandle = nullptr; } @@ -127,13 +127,13 @@ class RpcapdServerInitializer // Sizeof C-string includes the NULL terminator in the size. // For the purposes of this calculation the NULL terminator's inclusion in the size is used to simulate the space delimiter after the argument. args.reserve( - sizeof("-b") + ip.size() + + sizeof("rpcapd\\rpcapd.exe -b") + ip.size() + sizeof(" -p") + 5 /* The maximum digits a uint16_t can have is 5 */ + sizeof(" -n") - 1 /* Subtracts one as the last NULL terminator is not needed */ ); - args += "-b "; + args += "rpcapd\\rpcapd.exe -b "; args += ip; - args += " -p"; + args += " -p "; args += std::to_string(port); args += " -n"; @@ -171,17 +171,17 @@ class RpcapdServerInitializer &si, &pi )) - { + { DWORD errCode = GetLastError(); CloseHandle(m_JobHandle); throw std::runtime_error("Create process failed with error code: " + std::to_string(errCode)); - } + } m_ProcessHandle = pi.hProcess; CloseHandle(pi.hThread); // We don't need the thread handle, so we can close it. if (!AssignProcessToJobObject(m_JobHandle, m_ProcessHandle)) - { + { DWORD errCode = GetLastError(); killProcessAndCloseHandles(); throw std::runtime_error("Failed assigning process to job object with code: " + std::to_string(errCode)); From 4fbc4d7078eb478ff712ef57fcd3ba186ef0caab Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 13:41:54 +0300 Subject: [PATCH 04/13] Lint --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 33351be41d..44456d7f20 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -127,8 +127,8 @@ class RpcapdServerInitializer // Sizeof C-string includes the NULL terminator in the size. // For the purposes of this calculation the NULL terminator's inclusion in the size is used to simulate the space delimiter after the argument. args.reserve( - sizeof("rpcapd\\rpcapd.exe -b") + ip.size() + - sizeof(" -p") + 5 /* The maximum digits a uint16_t can have is 5 */ + + sizeof("rpcapd\\rpcapd.exe -b") + ip.size() + + sizeof(" -p") + 5 /* The maximum digits a uint16_t can have is 5 */ + sizeof(" -n") - 1 /* Subtracts one as the last NULL terminator is not needed */ ); args += "rpcapd\\rpcapd.exe -b "; From 21f46ed40c7f780b2663c9b7af74a8d42738ed98 Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 13:44:58 +0300 Subject: [PATCH 05/13] Removed unused include. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 44456d7f20..d31b3970fe 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -10,7 +10,6 @@ #include "../Common/GlobalTestArgs.h" #include "../Common/TestUtils.h" #include "../Common/PcapFileNamesDef.h" -#include #if defined(_WIN32) #include "PcapRemoteDevice.h" #include "PcapRemoteDeviceList.h" From 6c29b51282dbeab41e785efdca6eabc44af00254 Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 13:57:46 +0300 Subject: [PATCH 06/13] Lint --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index d31b3970fe..c7d7f7d5f1 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -148,7 +148,7 @@ class RpcapdServerInitializer JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimitInfo; ZeroMemory(&jobLimitInfo, sizeof(jobLimitInfo)); jobLimitInfo.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; - if (!SetInformationJobObject(m_JobHandle, JobObjectExtendedLimitInformation ,&jobLimitInfo, sizeof(jobLimitInfo))) + if (!SetInformationJobObject(m_JobHandle, JobObjectExtendedLimitInformation, &jobLimitInfo, sizeof(jobLimitInfo))) { DWORD errCode = GetLastError(); CloseHandle(m_JobHandle); From f21e7b0bd0dd5bea607339c6c73c7cd256961919 Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 14:06:12 +0300 Subject: [PATCH 07/13] Added comments. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index c7d7f7d5f1..dd057e7584 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -163,7 +163,10 @@ class RpcapdServerInitializer ZeroMemory( &pi, sizeof(pi) ); if (!CreateProcess ( TEXT(cmd.c_str()), - const_cast(TEXT(args.c_str())), // TODO: This can potentially cause access violation if Unicode version CreateProcessW is chosen. + // CreateProcessW (Unicode version) modifies the argument string inplace during internal processing. + // This can potentially cause access violation, if used with a compile time constant string, + // but since the current usage uses a heap allocated string, it should be fine. + const_cast(TEXT(args.c_str())), nullptr, nullptr, false, CREATE_NEW_CONSOLE, nullptr, nullptr, From 25ea15a5c974be7fcdd4df42b92089a08aedb6b9 Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 14:27:10 +0300 Subject: [PATCH 08/13] Lint --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index dd057e7584..9329fbfe47 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -163,8 +163,8 @@ class RpcapdServerInitializer ZeroMemory( &pi, sizeof(pi) ); if (!CreateProcess ( TEXT(cmd.c_str()), - // CreateProcessW (Unicode version) modifies the argument string inplace during internal processing. - // This can potentially cause access violation, if used with a compile time constant string, + // CreateProcessW (Unicode version) modifies the argument string inplace during internal processing. + // This can potentially cause access violation, if used with a compile time constant string, // but since the current usage uses a heap allocated string, it should be fine. const_cast(TEXT(args.c_str())), nullptr, nullptr, false, From 334a4d0ece20556dc8e799a57a55d60ef6b30f4a Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 19:01:10 +0300 Subject: [PATCH 09/13] Simplified zero memory call. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 9329fbfe47..1772260046 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -145,8 +145,7 @@ class RpcapdServerInitializer // Sets up the job limits so closing the job will automatically kill all processes assigned to the job. // This will prevent the subprocess continuing to live if the current process is killed without unwinding the stack (i.e. std::terminate is called), // as the OS itself will kill the subprocesses when the last job handle is closed. - JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimitInfo; - ZeroMemory(&jobLimitInfo, sizeof(jobLimitInfo)); + JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimitInfo{}; jobLimitInfo.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; if (!SetInformationJobObject(m_JobHandle, JobObjectExtendedLimitInformation, &jobLimitInfo, sizeof(jobLimitInfo))) { From a8316a9259122781ce9f11eafc0fed62534a56cf Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 19:45:56 +0300 Subject: [PATCH 10/13] Simplified arguments formatting --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 31 ++++++++++------------ 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 1772260046..638dc0ab98 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -10,6 +10,8 @@ #include "../Common/GlobalTestArgs.h" #include "../Common/TestUtils.h" #include "../Common/PcapFileNamesDef.h" +#include +#include #if defined(_WIN32) #include "PcapRemoteDevice.h" #include "PcapRemoteDeviceList.h" @@ -121,20 +123,17 @@ class RpcapdServerInitializer return; std::string cmd = "rpcapd\\rpcapd.exe"; - std::string args; - // Reserves the size of the arguments string in 1 allocation. - // Sizeof C-string includes the NULL terminator in the size. - // For the purposes of this calculation the NULL terminator's inclusion in the size is used to simulate the space delimiter after the argument. - args.reserve( - sizeof("rpcapd\\rpcapd.exe -b") + ip.size() + - sizeof(" -p") + 5 /* The maximum digits a uint16_t can have is 5 */ + - sizeof(" -n") - 1 /* Subtracts one as the last NULL terminator is not needed */ - ); - args += "rpcapd\\rpcapd.exe -b "; - args += ip; - args += " -p "; - args += std::to_string(port); - args += " -n"; + std::array args; + int res = std::snprintf(args.data(), args.size(), "rpcapd\\rpcapd.exe -b %s -p %d -n", ip.c_str(), port); + if (res < 0) + { + throw std::runtime_error("Error during string formatting"); + } + else if (res >= args.size()) + { + // We should never get here with reasonable input but you never know. + throw std::runtime_error("Buffer overflow. Are you sure the IP is a valid string?"); + } m_JobHandle = CreateJobObject(nullptr, nullptr); if (m_JobHandle == nullptr) @@ -163,9 +162,7 @@ class RpcapdServerInitializer if (!CreateProcess ( TEXT(cmd.c_str()), // CreateProcessW (Unicode version) modifies the argument string inplace during internal processing. - // This can potentially cause access violation, if used with a compile time constant string, - // but since the current usage uses a heap allocated string, it should be fine. - const_cast(TEXT(args.c_str())), + TEXT(args.data()), nullptr, nullptr, false, CREATE_NEW_CONSOLE, nullptr, nullptr, From 60233e26bfff1ff971493eb80fb5f19a83c1e23d Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 19:55:09 +0300 Subject: [PATCH 11/13] Added move ctors. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 638dc0ab98..962f760fbb 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -187,7 +187,22 @@ class RpcapdServerInitializer } RpcapdServerInitializer(const RpcapdServerInitializer&) = delete; + RpcapdServerInitializer(RpcapdServerInitializer&& other) noexcept + : m_ProcessHandle(other.m_ProcessHandle), m_JobHandle(other.m_JobHandle) + { + other.m_ProcessHandle = nullptr; + other.m_JobHandle = nullptr; + } RpcapdServerInitializer& operator=(const RpcapdServerInitializer&) = delete; + RpcapdServerInitializer& operator=(RpcapdServerInitializer&& other) noexcept + { + killProcessAndCloseHandles(); + m_ProcessHandle = other.m_ProcessHandle; + m_JobHandle = other.m_JobHandle; + other.m_ProcessHandle = nullptr; + other.m_JobHandle = nullptr; + return *this; + } ~RpcapdServerInitializer() { killProcessAndCloseHandles(); } From c6778277d295e221ca8d14b0f1cf30b24a1964fd Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Sun, 26 May 2024 19:55:57 +0300 Subject: [PATCH 12/13] Lint --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 962f760fbb..12a96bc712 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -187,7 +187,7 @@ class RpcapdServerInitializer } RpcapdServerInitializer(const RpcapdServerInitializer&) = delete; - RpcapdServerInitializer(RpcapdServerInitializer&& other) noexcept + RpcapdServerInitializer(RpcapdServerInitializer&& other) noexcept : m_ProcessHandle(other.m_ProcessHandle), m_JobHandle(other.m_JobHandle) { other.m_ProcessHandle = nullptr; From 519b35aba9447eb54d3f39170940e5bba2161846 Mon Sep 17 00:00:00 2001 From: Dimitar Krastev Date: Wed, 29 May 2024 18:24:03 +0300 Subject: [PATCH 13/13] Replaced hardcoded string with cmd variable. --- Tests/Pcap++Test/Tests/LiveDeviceTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp index 12a96bc712..0cf2ef4cc4 100644 --- a/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp +++ b/Tests/Pcap++Test/Tests/LiveDeviceTests.cpp @@ -124,7 +124,7 @@ class RpcapdServerInitializer std::string cmd = "rpcapd\\rpcapd.exe"; std::array args; - int res = std::snprintf(args.data(), args.size(), "rpcapd\\rpcapd.exe -b %s -p %d -n", ip.c_str(), port); + int res = std::snprintf(args.data(), args.size(), "%s -b %s -p %d -n", cmd.c_str(), ip.c_str(), port); if (res < 0) { throw std::runtime_error("Error during string formatting");