Fixed CORE-6441: Srp plugin keeps connection after database has been removed for ~10 seconds (SS and SC)

AlexPeshkoff · AlexPeshkoff · commit b4386db63b9c · 2020-11-16T17:36:36.000+03:00
diff --git a/src/auth/SecDbCache.cpp b/src/auth/SecDbCache.cpp
@@ -71,13 +71,23 @@ void PluginDatabases::getInstance(IPluginConfig* pluginConfig, RefPtr<CachedSecu
 
 	{ // guard scope
 		MutexLockGuard g(arrayMutex, FB_FUNCTION);
-		for (unsigned int i = 0; i < dbArray.getCount(); ++i)
+		for (unsigned int i = 0; i < dbArray.getCount(); )
 		{
 			if (secDbName == dbArray[i]->secureDbName)
 			{
-				instance = dbArray[i];
-				break;
+				CachedSecurityDatabase* fromCache = dbArray[i];
+				if (fromCache->secDb->test())
+				{
+					instance = fromCache;
+					break;
+				}
+				else
+				{
+					dbArray.remove(i);
+					continue;
+				}
 			}
+			++i;
 		}
 
 		if (!instance)
@@ -147,7 +157,7 @@ void PluginDatabases::handler(CachedSecurityDatabase* tgt)
 		const ISC_STATUS* status = st.begin();
 		if (status[0] == 1 && status[1] != isc_att_shutdown)
 		{
-			iscLogStatus("Legacy security database timer handler", status);
+			iscLogStatus("Security database timer handler", status);
 		}
 	}
 }
diff --git a/src/auth/SecDbCache.h b/src/auth/SecDbCache.h
@@ -49,6 +49,7 @@ class VSecDb
 	}
 
 	virtual bool lookup(void* inMsg, void* outMsg) = 0;
+	virtual bool test() = 0;
 };
 
 
diff --git a/src/auth/SecureRemotePassword/server/SrpServer.cpp b/src/auth/SecureRemotePassword/server/SrpServer.cpp
@@ -109,7 +109,8 @@ class SrpServer : public StdPlugin<IServerImpl<SrpServer, CheckStatusWrapper> >
 class SecurityDatabase : public VSecDb
 {
 public:
-	bool lookup(void* inMsg, void* outMsg)
+	// VSecDb implementation
+	bool lookup(void* inMsg, void* outMsg) override
 	{
 		FbLocalStatus status;
 
@@ -120,6 +121,14 @@ class SecurityDatabase : public VSecDb
 		return false;	// safe default
 	}
 
+	bool test() override
+	{
+		FbLocalStatus status;
+
+		att->ping(&status);
+		return !(status->getState() & IStatus::STATE_ERRORS);
+	}
+
 	// This 2 are needed to satisfy temporarily different calling requirements
 	static int shutdown(const int, const int, void*)
 	{
diff --git a/src/auth/SecurityDatabase/LegacyServer.cpp b/src/auth/SecurityDatabase/LegacyServer.cpp
@@ -146,7 +146,12 @@ class SecurityDatabaseServer FB_FINAL :
 class SecurityDatabase : public VSecDb
 {
 public:
-	bool lookup(void* inMsg, void* outMsg);
+	bool lookup(void* inMsg, void* outMsg) override;
+
+	bool test() override
+	{
+		return fb_ping(status, &lookup_db) == FB_SUCCESS;
+	}
 
 	// This 2 are needed to satisfy temporarily different calling requirements
 	static int shutdown(const int, const int, void*)
diff --git a/src/jrd/ExtEngineManager.cpp b/src/jrd/ExtEngineManager.cpp
@@ -603,8 +603,6 @@ ExtEngineManager::ExternalContextImpl::ExternalContextImpl(thread_db* tdbb,
 
 	clientCharSet = INTL_charset_lookup(tdbb, internalAttachment->att_client_charset)->getName();
 
-	internalAttachment->getStable()->addRef();
-
 	externalAttachment = MasterInterfacePtr()->registerAttachment
 		(internalAttachment->getProvider(), internalAttachment->getInterface());
 }

Original file line number	Diff line number	Diff line change
`@@ -71,13 +71,23 @@ void PluginDatabases::getInstance(IPluginConfig* pluginConfig, RefPtr<CachedSecu`
`71`	`71`
`72`	`72`	`{ // guard scope`
`73`	`73`	`MutexLockGuard g(arrayMutex, FB_FUNCTION);`
`74`		`- for (unsigned int i = 0; i < dbArray.getCount(); ++i)`
	`74`	`+ for (unsigned int i = 0; i < dbArray.getCount(); )`
`75`	`75`	`{`
`76`	`76`	`if (secDbName == dbArray[i]->secureDbName)`
`77`	`77`	`{`
`78`		`- instance = dbArray[i];`
`79`		`- break;`
	`78`	`+ CachedSecurityDatabase* fromCache = dbArray[i];`
	`79`	`+ if (fromCache->secDb->test())`
	`80`	`+ {`
	`81`	`+ instance = fromCache;`
	`82`	`+ break;`
	`83`	`+ }`
	`84`	`+ else`
	`85`	`+ {`
	`86`	`+ dbArray.remove(i);`
	`87`	`+ continue;`
	`88`	`+ }`
`80`	`89`	`}`
	`90`	`+ ++i;`
`81`	`91`	`}`
`82`	`92`
`83`	`93`	`if (!instance)`
`@@ -147,7 +157,7 @@ void PluginDatabases::handler(CachedSecurityDatabase* tgt)`
`147`	`157`	`const ISC_STATUS* status = st.begin();`
`148`	`158`	`if (status[0] == 1 && status[1] != isc_att_shutdown)`
`149`	`159`	`{`
`150`		`- iscLogStatus("Legacy security database timer handler", status);`
	`160`	`+ iscLogStatus("Security database timer handler", status);`
`151`	`161`	`}`
`152`	`162`	`}`
`153`	`163`	`}`
Original file line number	Diff line number	Diff line change
`@@ -49,6 +49,7 @@ class VSecDb`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`virtual bool lookup(void* inMsg, void* outMsg) = 0;`
	`52`	`+ virtual bool test() = 0;`
`52`	`53`	`};`
`53`	`54`
`54`	`55`
Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,8 @@ class SrpServer : public StdPlugin<IServerImpl<SrpServer, CheckStatusWrapper> >`
`109`	`109`	`class SecurityDatabase : public VSecDb`
`110`	`110`	`{`
`111`	`111`	`public:`
`112`		`- bool lookup(void* inMsg, void* outMsg)`
	`112`	`+ // VSecDb implementation`
	`113`	`+ bool lookup(void* inMsg, void* outMsg) override`
`113`	`114`	`{`
`114`	`115`	`FbLocalStatus status;`
`115`	`116`
`@@ -120,6 +121,14 @@ class SecurityDatabase : public VSecDb`
`120`	`121`	`return false; // safe default`
`121`	`122`	`}`
`122`	`123`
	`124`	`+ bool test() override`
	`125`	`+ {`
	`126`	`+ FbLocalStatus status;`
	`127`	`+`
	`128`	`+ att->ping(&status);`
	`129`	`+ return !(status->getState() & IStatus::STATE_ERRORS);`
	`130`	`+ }`
	`131`	`+`
`123`	`132`	`// This 2 are needed to satisfy temporarily different calling requirements`
`124`	`133`	`static int shutdown(const int, const int, void*)`
`125`	`134`	`{`
Original file line number	Diff line number	Diff line change
`@@ -603,8 +603,6 @@ ExtEngineManager::ExternalContextImpl::ExternalContextImpl(thread_db* tdbb,`
`603`	`603`
`604`	`604`	`clientCharSet = INTL_charset_lookup(tdbb, internalAttachment->att_client_charset)->getName();`
`605`	`605`
`606`		`- internalAttachment->getStable()->addRef();`
`607`		`-`
`608`	`606`	`externalAttachment = MasterInterfacePtr()->registerAttachment`
`609`	`607`	`(internalAttachment->getProvider(), internalAttachment->getInterface());`
`610`	`608`	`}`