Skip to content

Commit

Permalink
Fix use-after-free crash in SQLite extension (#481)
Browse files Browse the repository at this point in the history
When the server crashed and the process got terminated, the SqDriver
instance was killed first (e.g. by atexit). SqDatabase tries to access
SqDriver in its destructor.
This patch tells SqDatabase to not use anything from SqDriver anymore
after SqDriver got destroyed.

Next to that, the clientprefs extension relied on the IDatabase pointer
being valid to get the driver pointer. Cache the pointer, so the dbi
system still knows the IDBThreadOperation belonged to the now gone
driver, even after the database object is gone.
  • Loading branch information
peace-maker authored and asherkin committed Oct 3, 2016
1 parent 2deaa66 commit 47eb7d6
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 1 deletion.
5 changes: 4 additions & 1 deletion extensions/clientprefs/query.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void TQueryOp::RunThreadPart()

IDBDriver *TQueryOp::GetDriver()
{
return m_database->GetDriver();
return m_driver;
}

IdentityToken_t *TQueryOp::GetOwner()
Expand All @@ -114,6 +114,7 @@ TQueryOp::TQueryOp(enum querytype type, int serial)
m_type = type;
m_serial = serial;
m_database = NULL;
m_driver = NULL;
m_insertId = -1;
m_pResult = NULL;
}
Expand All @@ -123,6 +124,7 @@ TQueryOp::TQueryOp(enum querytype type, Cookie *cookie)
m_type = type;
m_pCookie = cookie;
m_database = NULL;
m_driver = NULL;
m_insertId = -1;
m_pResult = NULL;
m_serial = 0;
Expand All @@ -131,6 +133,7 @@ TQueryOp::TQueryOp(enum querytype type, Cookie *cookie)
void TQueryOp::SetDatabase(IDatabase *db)
{
m_database = db;
m_driver = m_database->GetDriver();
}

bool TQueryOp::BindParamsAndRun()
Expand Down
1 change: 1 addition & 0 deletions extensions/clientprefs/query.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class TQueryOp : public IDBThreadOperation

private:
IDatabase *m_database;
IDBDriver *m_driver;
IQuery *m_pResult;

/* Query type */
Expand Down
4 changes: 4 additions & 0 deletions extensions/sqlite/driver/SqDatabase.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ class SqDatabase
bool SetCharacterSet(const char *characterset);
public:
sqlite3 *GetDb();
void PrepareForForcedShutdown()
{
m_Persistent = false;
}
private:
sqlite3 *m_sq3;
ke::AutoPtr<ke::Mutex> m_FullLock;
Expand Down
27 changes: 27 additions & 0 deletions extensions/sqlite/driver/SqDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,32 @@ SqDriver::SqDriver()
{
m_Handle = BAD_HANDLE;
m_bThreadSafe = false;
m_bShutdown = false;
}

// The extension got unloaded. Remove any open database now to avoid a use-after-free
// of g_SqDriver in SqDatabase's destructor.
SqDriver::~SqDriver()
{
ke::AutoLock lock(&m_OpenLock);

List<SqDbInfo>::iterator iter;
SqDatabase *sqdb;
for (iter = m_Cache.begin(); iter != m_Cache.end(); iter++)
{
// Don't let SqDatabase try to remove itself from m_Cache
// now that we're gone.
sqdb = (SqDatabase *)(*iter).db;
sqdb->PrepareForForcedShutdown();

iter = m_Cache.erase(iter);
}

if (!m_bShutdown)
{
dbi->RemoveDriver(&g_SqDriver);
Shutdown();
}
}

void SqDriver::Initialize()
Expand All @@ -76,6 +102,7 @@ void SqDriver::Initialize()

void SqDriver::Shutdown()
{
m_bShutdown = true;
if (m_bThreadSafe)
{
sqlite3_enable_shared_cache(0);
Expand Down
2 changes: 2 additions & 0 deletions extensions/sqlite/driver/SqDriver.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class SqDriver : public IDBDriver
{
public:
SqDriver();
~SqDriver();
void Initialize();
void Shutdown();
public:
Expand All @@ -74,6 +75,7 @@ class SqDriver : public IDBDriver
ke::Mutex m_OpenLock;
List<SqDbInfo> m_Cache;
bool m_bThreadSafe;
bool m_bShutdown;
};

extern SqDriver g_SqDriver;
Expand Down

0 comments on commit 47eb7d6

Please sign in to comment.