Skip to content

Commit 41a8fc5

Browse files
AlexPeshkoffAlexPeshkoff
committed
Fixed CORE-6450: Races in cache of opened security databases
1 parent 61cac96 commit 41a8fc5

5 files changed

Lines changed: 50 additions & 52 deletions

File tree

‎src/auth/SecDbCache.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ void CachedSecurityDatabase::handler()
5151
}
5252

5353

54-
void PluginDatabases::getInstance(IPluginConfig* pluginConfig, RefPtr<CachedSecurityDatabase>& instance)
54+
void PluginDatabases::getInstance(IPluginConfig* pluginConfig, CachedSecurityDatabase::Instance& instance)
5555
{
5656
// Determine sec.db name based on existing config
5757
PathName secDbName;
@@ -78,7 +78,7 @@ void PluginDatabases::getInstance(IPluginConfig* pluginConfig, RefPtr<CachedSecu
7878
CachedSecurityDatabase* fromCache = dbArray[i];
7979
if (fromCache->secDb->test())
8080
{
81-
instance = fromCache;
81+
instance.set(fromCache);
8282
break;
8383
}
8484
else
@@ -92,7 +92,7 @@ void PluginDatabases::getInstance(IPluginConfig* pluginConfig, RefPtr<CachedSecu
9292

9393
if (!instance)
9494
{
95-
instance = FB_NEW CachedSecurityDatabase(this, secDbName);
95+
instance.set(FB_NEW CachedSecurityDatabase(this, secDbName));
9696
instance->addRef();
9797
secDbName.copyTo(instance->secureDbName, sizeof(instance->secureDbName));
9898
dbArray.add(instance);

‎src/auth/SecDbCache.h‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,33 @@ class CachedSecurityDatabase FB_FINAL
7474
Firebird::Mutex mutex;
7575
Firebird::AutoPtr<VSecDb> secDb;
7676
PluginDatabases* list;
77+
78+
public:
79+
// Related RAII holder
80+
class Instance : public Firebird::RefPtr<CachedSecurityDatabase>
81+
{
82+
public:
83+
Instance()
84+
{ }
85+
86+
void set(CachedSecurityDatabase* db)
87+
{
88+
fb_assert(!hasData());
89+
fb_assert(db);
90+
91+
assign(db);
92+
(*this)->mutex.enter(FB_FUNCTION);
93+
}
94+
95+
~Instance()
96+
{
97+
if (hasData())
98+
{
99+
(*this)->mutex.leave();
100+
(*this)->close();
101+
}
102+
}
103+
};
77104
};
78105

79106
class PluginDatabases
@@ -88,7 +115,7 @@ class PluginDatabases
88115
Firebird::Mutex arrayMutex;
89116

90117
public:
91-
void getInstance(Firebird::IPluginConfig* pluginConfig, Firebird::RefPtr<CachedSecurityDatabase>& instance);
118+
void getInstance(Firebird::IPluginConfig* pluginConfig, CachedSecurityDatabase::Instance& instance);
92119
int shutdown();
93120
void handler(CachedSecurityDatabase* tgt);
94121
};

‎src/auth/SecureRemotePassword/server/SrpServer.cpp‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -287,28 +287,19 @@ int SrpServer::authenticate(CheckStatusWrapper* status, IServerBlock* sb, IWrite
287287
messages.param->loginNull = 0;
288288
messages.data.clear();
289289

290-
{ // reference & mutex scope scope
290+
{ // instance RAII scope
291+
CachedSecurityDatabase::Instance instance;
292+
291293
// Get database block from cache
292-
RefPtr<CachedSecurityDatabase> instance;
293294
instances->getInstance(iParameter, instance);
295+
secDbName = instance->secureDbName;
294296

295-
try
296-
{
297-
MutexLockGuard g(instance->mutex, FB_FUNCTION);
298-
299-
secDbName = instance->secureDbName;
300-
if (!instance->secDb)
301-
instance->secDb = FB_NEW SecurityDatabase(instance->secureDbName, cryptCallback);
302-
303-
instance->secDb->lookup(messages.param.getData(), messages.data.getData());
304-
}
305-
catch(const Exception&)
306-
{
307-
instance->close();
308-
throw;
309-
}
297+
// Create SecurityDatabase if needed
298+
if (!instance->secDb)
299+
instance->secDb = FB_NEW SecurityDatabase(instance->secureDbName, cryptCallback);
310300

311-
instance->close();
301+
// Lookup
302+
instance->secDb->lookup(messages.param.getData(), messages.data.getData());
312303
}
313304
HANDSHAKE_DEBUG(fprintf(stderr, "Srv: SRP1: Executed statement\n"));
314305

‎src/auth/SecurityDatabase/LegacyServer.cpp‎

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -330,32 +330,20 @@ int SecurityDatabaseServer::authenticate(CheckStatusWrapper* status, IServerBloc
330330
bool found = false;
331331
char pw1[MAX_LEGACY_PASSWORD_LENGTH + 1];
332332
PathName secureDbName;
333-
{ // reference & mutex scope scope
333+
{ // instance scope
334334
// Get database block from cache
335-
RefPtr<CachedSecurityDatabase> instance;
335+
CachedSecurityDatabase::Instance instance;
336336
instances->getInstance(iParameter, instance);
337337

338-
try
339-
{
340-
MutexLockGuard g(instance->mutex, FB_FUNCTION);
341-
342-
secureDbName = instance->secureDbName;
343-
if (!instance->secDb)
344-
instance->secDb = FB_NEW SecurityDatabase(instance->secureDbName);
345-
346-
user_name uname; // user name buffer
347-
login.copyTo(uname, sizeof uname);
348-
user_record user_block; // user record
349-
found = instance->secDb->lookup(uname, &user_block);
350-
fb_utils::copy_terminate(pw1, user_block.password, MAX_LEGACY_PASSWORD_LENGTH + 1);
351-
}
352-
catch(const Exception&)
353-
{
354-
instance->close();
355-
throw;
356-
}
338+
secureDbName = instance->secureDbName;
339+
if (!instance->secDb)
340+
instance->secDb = FB_NEW SecurityDatabase(instance->secureDbName);
357341

358-
instance->close();
342+
user_name uname; // user name buffer
343+
login.copyTo(uname, sizeof uname);
344+
user_record user_block; // user record
345+
found = instance->secDb->lookup(uname, &user_block);
346+
fb_utils::copy_terminate(pw1, user_block.password, MAX_LEGACY_PASSWORD_LENGTH + 1);
359347
}
360348
if (!found)
361349
{

‎src/common/classes/RefCounted.h‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -167,14 +167,6 @@ namespace Firebird
167167
return ptr;
168168
}
169169

170-
/* NS: you cannot have operator bool here. It creates ambiguity with
171-
operator T* with some of the compilers (at least VS2003)
172-
173-
operator bool() const
174-
{
175-
return ptr ? true : false;
176-
}*/
177-
178170
bool hasData() const
179171
{
180172
return ptr ? true : false;

0 commit comments

Comments
 (0)