Fixed #7197: Segfault in linux CS after successful detach from database

Author: AlexPeshkoff

src/jrd/jrd.cpp

@@ -111,6 +111,7 @@
 #include "../yvalve/why_proto.h"
 #include "../jrd/flags.h"
 #include "../jrd/Mapping.h"
+#include "../jrd/ThreadCollect.h"
 
 #include "../jrd/Database.h"
 
@@ -124,6 +125,7 @@
 #include "../common/classes/fb_tls.h"
 #include "../common/classes/ClumpletWriter.h"
 #include "../common/classes/RefMutex.h"
+#include "../common/classes/semaphore.h"
 #include "../common/utils_proto.h"
 #include "../jrd/DebugInterface.h"
 #include "../jrd/CryptoManager.h"
@@ -449,6 +451,16 @@ namespace
 {
 	using Jrd::Attachment;
 
+	// Required to sync attachment shutdown threads with provider shutdown
+	GlobalPtr<ThreadCollect> shutThreadCollect;
+
+	struct AttShutParams
+	{
+		Semaphore thdStartedSem;
+		Thread::Handle thrHandle;
+		AttachmentsRefHolder* attachments;
+	};
+
 	// Flag engineShutdown guarantees that no new attachment is created after setting it
 	// and helps avoid more than 1 shutdown threads running simultaneously.
 	bool engineShutdown = false;
@@ -4130,59 +4142,65 @@ void JProvider::shutdown(CheckStatusWrapper* status, unsigned int timeout, const
  **************************************/
 	try
 	{
-		MutexLockGuard guard(shutdownMutex, FB_FUNCTION);
-
-		if (engineShutdown)
-		{
-			return;
-		}
 		{ // scope
-			MutexLockGuard guard(newAttachmentMutex, FB_FUNCTION);
-			engineShutdown = true;
-		}
+			MutexLockGuard guard(shutdownMutex, FB_FUNCTION);
 
-		ThreadContextHolder tdbb;
+			if (engineShutdown)
+			{
+				return;
+			}
 
-		ULONG attach_count, database_count, svc_count;
-		JRD_enum_attachments(NULL, attach_count, database_count, svc_count);
+			{ // scope
+				MutexLockGuard guard(newAttachmentMutex, FB_FUNCTION);
+				engineShutdown = true;
+			}
 
-		if (attach_count > 0 || svc_count > 0)
-		{
-			gds__log("Shutting down the server with %d active connection(s) to %d database(s), "
-					 "%d active service(s)",
-				attach_count, database_count, svc_count);
-		}
+			ThreadContextHolder tdbb;
 
-		if (reason == fb_shutrsn_exit_called)
-		{
-			// Starting threads may fail when task is going to close.
-			// This happens at least with some microsoft C runtimes.
-			// If people wish to have timeout, they should better call fb_shutdown() themselves.
-			// Therefore:
-			timeout = 0;
-		}
+			ULONG attach_count, database_count, svc_count;
+			JRD_enum_attachments(NULL, attach_count, database_count, svc_count);
 
-		if (timeout)
-		{
-			Semaphore shutdown_semaphore;
+			if (attach_count > 0 || svc_count > 0)
+			{
+				gds__log("Shutting down the server with %d active connection(s) to %d database(s), "
+						 "%d active service(s)",
+					attach_count, database_count, svc_count);
+			}
 
-			Thread::Handle h;
-			Thread::start(shutdown_thread, &shutdown_semaphore, THREAD_medium, &h);
+			if (reason == fb_shutrsn_exit_called)
+			{
+				// Starting threads may fail when task is going to close.
+				// This happens at least with some microsoft C runtimes.
+				// If people wish to have timeout, they should better call fb_shutdown() themselves.
+				// Therefore:
+				timeout = 0;
+			}
 
-			if (!shutdown_semaphore.tryEnter(0, timeout))
-				waitForShutdown(shutdown_semaphore);
+			if (timeout)
+			{
+				Semaphore shutdown_semaphore;
 
-			Thread::waitForCompletion(h);
-		}
-		else
-		{
-			shutdown_thread(NULL);
+				Thread::Handle h;
+				Thread::start(shutdown_thread, &shutdown_semaphore, THREAD_medium, &h);
+
+				if (!shutdown_semaphore.tryEnter(0, timeout))
+					waitForShutdown(shutdown_semaphore);
+
+				Thread::waitForCompletion(h);
+			}
+			else
+			{
+				shutdown_thread(NULL);
+			}
+
+			// Do not put it into separate shutdown thread - during shutdown of TraceManager
+			// PluginManager wants to lock a mutex, which is sometimes already locked in current thread
+			TraceManager::shutdown();
+			shutdownMappingIpc();
 		}
 
-		// Do not put it into separate shutdown thread - during shutdown of TraceManager
-		// PluginManager wants to lock a mutex, which is sometimes already locked in current thread
-		TraceManager::shutdown();
-		shutdownMappingIpc();
+		// Wait for completion of all shutdown threads
+		shutThreadCollect->join();
 	}
 	catch (const Exception& ex)
 	{
@@ -7478,22 +7496,26 @@ namespace
 		ThreadModuleRef thdRef(attachmentShutdownThread, &engineShutdown);
 #endif
 
+		AttShutParams* params = static_cast<AttShutParams*>(arg);
+		AttachmentsRefHolder* attachments = params->attachments;
+		Thread::Handle th = params->thrHandle;
+		fb_assert(th);
+
 		try
 		{
-			MutexLockGuard guard(shutdownMutex, FB_FUNCTION);
-			if (engineShutdown)
-			{
-				// Shutdown was done, all attachments are gone
-				return 0;
-			}
+			shutThreadCollect->running(th);
+			params->thdStartedSem.release();
 
-			shutdownAttachments(static_cast<AttachmentsRefHolder*>(arg), false);
+			MutexLockGuard guard(shutdownMutex, FB_FUNCTION);
+			if (!engineShutdown)
+				shutdownAttachments(attachments, false);
 		}
 		catch (const Exception& ex)
 		{
 			iscLogException("attachmentShutdownThread", ex);
 		}
 
+		shutThreadCollect->ending(th);
 		return 0;
 	}
 } // anonymous namespace
@@ -8183,7 +8205,12 @@ void JRD_shutdown_attachment(Attachment* attachment)
 		if (!(attachment->att_flags & ATT_shutdown))
 			attachment->signalShutdown();
 
-		Thread::start(attachmentShutdownThread, queue.release(), THREAD_high);
+		AttShutParams params;
+		params.attachments = queue;
+		Thread::start(attachmentShutdownThread, &params, THREAD_high, &params.thrHandle);
+		queue.release();
+		shutThreadCollect->houseKeeping();
+		params.thdStartedSem.enter();
 	}
 	catch (const Exception&)
 	{} // no-op
@@ -8239,7 +8266,12 @@ void JRD_shutdown_attachments(Database* dbb)
 					attachment->signalShutdown();
 			}
 
-			Thread::start(attachmentShutdownThread, queue.release(), THREAD_high);
+			AttShutParams params;
+			params.attachments = queue;
+			Thread::start(attachmentShutdownThread, &params, THREAD_high, &params.thrHandle);
+			queue.release();
+			shutThreadCollect->houseKeeping();
+			params.thdStartedSem.enter();
 		}
 	}
 	catch (const Exception&)

src/jrd/svc.cpp

@@ -77,6 +77,7 @@
 #include "../utilities/nbackup/nbkswi.h"
 #include "../jrd/trace/traceswi.h"
 #include "../jrd/val_proto.h"
+#include "../jrd/ThreadCollect.h"
 
 #ifdef HAVE_SYS_TYPES_H
 #include <sys/types.h>
@@ -114,6 +115,7 @@
 
 
 using namespace Firebird;
+using namespace Jrd;
 
 const int SVC_user_dba			= 2;
 const int SVC_user_any			= 1;
@@ -132,63 +134,10 @@ namespace {
 	GlobalPtr<Mutex> globalServicesMutex;
 
 	// All that we need to shutdown service threads when shutdown in progress
-	typedef Array<Jrd::Service*> AllServices;
+	typedef Array<Service*> AllServices;
 	GlobalPtr<AllServices> allServices;	// protected by globalServicesMutex
 	volatile bool svcShutdown = false;
 
-	class ThreadCollect
-	{
-	public:
-		ThreadCollect(MemoryPool& p)
-			: threads(p)
-		{ }
-
-		void join()
-		{
-			// join threads to be sure they are gone when shutdown is complete
-			// no need locking something cause this is expected to run when services are closing
-			waitFor(threads);
-		}
-
-		void add(Thread::Handle& h)
-		{
-			// put thread into completion wait queue when it finished running
-			MutexLockGuard g(threadsMutex, FB_FUNCTION);
-			threads.add(h);
-		}
-
-		void houseKeeping()
-		{
-			if (!threads.hasData())
-				return;
-
-			// join finished threads
-			AllThreads t;
-			{ // mutex scope
-				MutexLockGuard g(threadsMutex, FB_FUNCTION);
-				t.assign(threads);
-				threads.clear();
-			}
-
-			waitFor(t);
-		}
-
-	private:
-		typedef Array<Thread::Handle> AllThreads;
-
-		static void waitFor(AllThreads& thr)
-		{
-			while (thr.hasData())
-			{
-				Thread::Handle h(thr.pop());
-				Thread::waitForCompletion(h);
-			}
-		}
-
-		AllThreads threads;
-		Mutex threadsMutex;
-	};
-
 	GlobalPtr<ThreadCollect> threadCollect;
 
 	void spbVersionError()
@@ -200,8 +149,6 @@ namespace {
 } // anonymous namespace
 
 
-using namespace Jrd;
-
 Service::Validate::Validate(Service* svc)
 	: sharedGuard(globalServicesMutex, FB_FUNCTION)
 {
@@ -1983,7 +1930,7 @@ THREAD_ENTRY_DECLARE Service::run(THREAD_ENTRY_PARAM arg)
 		svc->finish(SVC_finished);
 
 		if (thrHandle)
-			threadCollect->add(thrHandle);
+			threadCollect->ending(thrHandle);
 	}
 	catch (const Exception& ex)
 	{