zenserverprocess hardening (#61)

* verify running process before creating event
* make sure we don't signal/wait for a zenserver instance that we did not wait for to get ready

4 files changed, 53 insertions, 25 deletions

diff --git a/src/zencore/thread.cpp b/src/zencore/thread.cpp
index cb3aced33..06093d6b0 100644
--- a/src/zencore/thread.cpp
+++ b/src/zencore/thread.cpp
@@ -326,6 +326,7 @@ NamedEvent::NamedEvent(std::string_view EventName)
 	Packed |= intptr_t(Fd) & 0xffff'ffff;
 	m_EventHandle = (void*)Packed;
 #endif
+	ZEN_ASSERT(m_EventHandle != nullptr);
 }
 
 NamedEvent::~NamedEvent()
@@ -365,6 +366,7 @@ NamedEvent::Close()
 void
 NamedEvent::Set()
 {
+	ZEN_ASSERT(m_EventHandle != nullptr);
 #if ZEN_PLATFORM_WINDOWS
 	SetEvent(m_EventHandle);
 #elif ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
@@ -376,6 +378,7 @@ NamedEvent::Set()
 bool
 NamedEvent::Wait(int TimeoutMs)
 {
+	ZEN_ASSERT(m_EventHandle != nullptr);
 #if ZEN_PLATFORM_WINDOWS
 	const DWORD Timeout = (TimeoutMs < 0) ? INFINITE : TimeoutMs;
 
diff --git a/src/zenserver-test/zenserver-test.cpp b/src/zenserver-test/zenserver-test.cpp
index d6671c885..4675ede38 100644
--- a/src/zenserver-test/zenserver-test.cpp
+++ b/src/zenserver-test/zenserver-test.cpp
@@ -2551,19 +2551,25 @@ public:
 		for (int i = 0; i < m_ServerCount; ++i)
 		{
 			auto& Instance = m_Instances[i];
-
-			Instance = std::make_unique<ZenServerInstance>(TestEnv);
+			Instance	   = std::make_unique<ZenServerInstance>(TestEnv);
 			Instance->SetTestDir(TestEnv.CreateNewTestDir());
+		}
 
+		for (int i = 0; i < m_ServerCount; ++i)
+		{
+			auto& Instance = m_Instances[i];
 			Callback(*Instance);
-
-			Instance->SpawnServer(TestEnv.GetNewPortNumber(), AdditionalServerArgs);
 		}
 
 		for (int i = 0; i < m_ServerCount; ++i)
 		{
 			auto& Instance = m_Instances[i];
+			Instance->SpawnServer(TestEnv.GetNewPortNumber(), AdditionalServerArgs);
+		}
 
+		for (int i = 0; i < m_ServerCount; ++i)
+		{
+			auto&	 Instance	= m_Instances[i];
 			uint16_t PortNumber = Instance->WaitUntilReady();
 			CHECK_MESSAGE(PortNumber != 0, Instance->GetLogOutput());
 		}
diff --git a/src/zenutil/include/zenutil/zenserverprocess.h b/src/zenutil/include/zenutil/zenserverprocess.h
index f7204fb43..8aa4a2773 100644
--- a/src/zenutil/include/zenutil/zenserverprocess.h
+++ b/src/zenutil/include/zenutil/zenserverprocess.h
@@ -67,7 +67,7 @@ struct ZenServerInstance
 	~ZenServerInstance();
 
 	int				   Shutdown();
-	void			   SignalShutdown();
+	bool			   SignalShutdown();
 	uint16_t		   WaitUntilReady();
 	[[nodiscard]] bool WaitUntilReady(int Timeout);
 	void			   EnableTermination() { m_Terminate = true; }
@@ -107,17 +107,17 @@ struct ZenServerInstance
 	uint16_t	GetBasePort() const { return m_BasePort; }
 
 private:
-	ZenServerEnvironment& m_Env;
-	ProcessHandle		  m_Process;
-	NamedEvent			  m_ReadyEvent;
-	NamedEvent			  m_ShutdownEvent;
-	bool				  m_Terminate		  = false;
-	bool				  m_ShutdownOnDestroy = true;
-	std::filesystem::path m_TestDir;
-	uint16_t			  m_BasePort = 0;
-	std::optional<int>	  m_OwnerPid;
-	std::string			  m_Name;
-	std::filesystem::path m_OutputCapturePath;
+	ZenServerEnvironment&		m_Env;
+	ProcessHandle				m_Process;
+	NamedEvent					m_ReadyEvent;
+	std::unique_ptr<NamedEvent> m_ShutdownEvent;
+	bool						m_Terminate			= false;
+	bool						m_ShutdownOnDestroy = true;
+	std::filesystem::path		m_TestDir;
+	uint16_t					m_BasePort = 0;
+	std::optional<int>			m_OwnerPid;
+	std::string					m_Name;
+	std::filesystem::path		m_OutputCapturePath;
 
 	void CreateShutdownEvent(int BasePort);
 	void SpawnServer(int BasePort, std::string_view AdditionalServerArgs, int WaitTimeoutMs);
diff --git a/src/zenutil/zenserverprocess.cpp b/src/zenutil/zenserverprocess.cpp
index 34eec9790..26c72fc9b 100644
--- a/src/zenutil/zenserverprocess.cpp
+++ b/src/zenutil/zenserverprocess.cpp
@@ -499,7 +499,7 @@ ZenServerEnvironment::CreateNewTestDir()
 	using namespace std::literals;
 
 	ExtendableWideStringBuilder<256> TestDir;
-	TestDir << "test"sv << int64_t(++ZenServerTestCounter);
+	TestDir << "test"sv << int64_t(ZenServerTestCounter.fetch_add(1));
 
 	std::filesystem::path TestPath = m_TestBaseDir / TestDir.c_str();
 	ZEN_ASSERT(!std::filesystem::exists(TestPath));
@@ -544,10 +544,18 @@ ZenServerInstance::~ZenServerInstance()
 	}
 }
 
-void
+bool
 ZenServerInstance::SignalShutdown()
 {
-	m_ShutdownEvent.Set();
+	if (m_ShutdownEvent)
+	{
+		m_ShutdownEvent->Set();
+		return true;
+	}
+	else
+	{
+		return false;
+	}
 }
 
 int
@@ -562,7 +570,6 @@ ZenServerInstance::Shutdown()
 				ZEN_INFO("Terminating zenserver process {}", m_Name);
 				int ExitCode = 111;
 				m_Process.Terminate(ExitCode);
-				m_Process.Reset();
 				ZEN_DEBUG("zenserver process {} ({}) terminated", m_Name, m_Process.Pid());
 				return ExitCode;
 			}
@@ -571,9 +578,16 @@ ZenServerInstance::Shutdown()
 				if (m_Process.IsRunning())
 				{
 					ZEN_DEBUG("Requesting zenserver process {} ({}) to shut down", m_Name, m_Process.Pid());
-					SignalShutdown();
+					if (!SignalShutdown())
+					{
+						ZEN_INFO("Terminating zenserver process as we did not wait for it to get ready {}", m_Name);
+						int ExitCode = 111;
+						m_Process.Terminate(ExitCode);
+						ZEN_DEBUG("zenserver process {} ({}) terminated", m_Name, m_Process.Pid());
+						return ExitCode;
+					}
 					ZEN_DEBUG("Waiting for zenserver process {} ({}) to shut down", m_Name, m_Process.Pid());
-					while (!m_Process.Wait(5000))
+					while (!m_Process.Wait(1000))
 					{
 						ZEN_WARN("Waiting for zenserver process {} ({}) timed out", m_Name, m_Process.Pid());
 					}
@@ -720,8 +734,7 @@ ZenServerInstance::CreateShutdownEvent(int BasePort)
 	ExtendableStringBuilder<32> ChildShutdownEventName;
 	ChildShutdownEventName << "Zen_" << BasePort;
 	ChildShutdownEventName << "_Shutdown";
-	NamedEvent ChildShutdownEvent{ChildShutdownEventName};
-	m_ShutdownEvent = std::move(ChildShutdownEvent);
+	m_ShutdownEvent = std::make_unique<NamedEvent>(ChildShutdownEventName);
 }
 
 void
@@ -774,7 +787,7 @@ ZenServerInstance::Detach()
 	if (m_Process.IsValid())
 	{
 		m_Process.Reset();
-		m_ShutdownEvent.Close();
+		m_ShutdownEvent.reset();
 	}
 }
 
@@ -862,8 +875,14 @@ ZenServerInstance::OnServerReady()
 		// TODO: return success/error code instead?
 		throw std::runtime_error("no server entry found");
 	}
+	ZEN_ASSERT(Entry->IsReady());
 
 	m_BasePort = Entry->EffectiveListenPort;
+	ZEN_ASSERT(m_BasePort != 0);
+	if (!IsProcessRunning(Entry->Pid.load()))
+	{
+		throw std::runtime_error("server no longer running");
+	}
 	CreateShutdownEvent(m_BasePort);
 
 	ZEN_DEBUG("Server '{}' is ready on port {}", m_Name, m_BasePort);