From 1e80174c8f8e21ad245902f4ea89c0df2c0dd88e Mon Sep 17 00:00:00 2001
From: Piotr Bartosiewicz
Date: Fri, 23 Jan 2015 13:45:59 +0100
Subject: [PATCH] Fix potential deadlock; FromKVJsonVisitor use db transaction
[Bug/Feature] Invalid mutexes lock sequence in kv store transaction.
Missing db transaction in FromKVJsonVisitor.
[Cause] N/A
[Solution] N/A
[Verification] Build, run tests
Change-Id: Ie44dbfe5ad86c9cbf0e19a162a267676b05f1aed
---
src/config/from-kvjson-visitor.hpp | 17 ++++++++++++-----
src/config/kvstore.cpp | 33 ++++++++++++++-------------------
src/config/kvstore.hpp | 11 ++++++-----
3 files changed, 32 insertions(+), 29 deletions(-)
diff --git a/src/config/from-kvjson-visitor.hpp b/src/config/from-kvjson-visitor.hpp
index c355fc2..2e386b5 100644
--- a/src/config/from-kvjson-visitor.hpp
+++ b/src/config/from-kvjson-visitor.hpp
@@ -38,6 +38,7 @@ public:
: mStorePtr(new KVStore(db))
, mKeyPrefix(prefix)
, mIsUnion(false)
+ , mTransaction(mStorePtr->getTransaction())
{
mObject = json_tokener_parse(json.c_str());
if (mObject == nullptr) {
@@ -54,7 +55,8 @@ public:
mObject(v.mObject ? json_object_get(v.mObject) : nullptr),
mStorePtr(v.mStorePtr),
mKeyPrefix(v.mKeyPrefix),
- mIsUnion(v.mIsUnion)
+ mIsUnion(v.mIsUnion),
+ mTransaction(v.mTransaction)
{
}
FromKVJsonVisitor& operator=(const FromKVJsonVisitor&) = delete;
@@ -69,12 +71,15 @@ private:
std::string mKeyPrefix;
json_object* mObject;
bool mIsUnion;
+ KVStore::Transaction mTransaction;
// create visitor for field object (visitable object)
FromKVJsonVisitor(const FromKVJsonVisitor& v, const std::string& name, const bool isUnion) :
- mStorePtr(v.mStorePtr), mIsUnion(isUnion || v.mIsUnion)
+ mStorePtr(v.mStorePtr),
+ mKeyPrefix(key(v.mKeyPrefix, name)),
+ mIsUnion(isUnion || v.mIsUnion),
+ mTransaction(v.mTransaction)
{
- mKeyPrefix = key(v.mKeyPrefix, name);
json_object* object = nullptr;
if (v.mObject && !json_object_object_get_ex(v.mObject, name.c_str(), &object)) {
if (!mIsUnion)
@@ -85,9 +90,11 @@ private:
// create visitor for vector i-th element (visitable object)
FromKVJsonVisitor(const FromKVJsonVisitor& v, int i, const bool isUnion) :
- mStorePtr(v.mStorePtr), mIsUnion(isUnion || v.mIsUnion)
+ mStorePtr(v.mStorePtr),
+ mKeyPrefix(key(v.mKeyPrefix, std::to_string(i))),
+ mIsUnion(isUnion || v.mIsUnion),
+ mTransaction(v.mTransaction)
{
- mKeyPrefix = key(v.mKeyPrefix, std::to_string(i));
json_object* object = nullptr;
if (v.mObject) {
object = json_object_array_get_idx(v.mObject, i);
diff --git a/src/config/kvstore.cpp b/src/config/kvstore.cpp
index a9d3fb6..476d182 100644
--- a/src/config/kvstore.cpp
+++ b/src/config/kvstore.cpp
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
namespace config {
@@ -96,38 +97,39 @@ void sqliteEscapeStr(::sqlite3_context* context, int argc, ::sqlite3_value** val
} // namespace
struct KVStore::TransactionImpl {
- TransactionImpl(KVStore* kvstorePtr)
- : mKVStorePtr(kvstorePtr)
+ TransactionImpl(KVStore& kvStore)
+ : mLock(kvStore.mMutex)
+ , mKVStore(kvStore)
{
- mKVStorePtr->mConnMtx.lock();
- mKVStorePtr->mConn.exec("BEGIN EXCLUSIVE TRANSACTION");
+ mKVStore.mConn.exec("BEGIN EXCLUSIVE TRANSACTION");
}
~TransactionImpl()
{
+ // be aware of a bug in gcc with uncaught_exception and rethrow_exception
if (std::uncaught_exception()) {
try {
- mKVStorePtr->mConn.exec("ROLLBACK TRANSACTION");
+ mKVStore.mConn.exec("ROLLBACK TRANSACTION");
} catch (ConfigException&) {}
} else {
try {
- mKVStorePtr->mConn.exec("COMMIT TRANSACTION");
+ mKVStore.mConn.exec("COMMIT TRANSACTION");
} catch (ConfigException&) {}
}
- mKVStorePtr->mConnMtx.unlock();
}
private:
- config::KVStore* mKVStorePtr;
+ config::KVStore::Lock mLock;
+ config::KVStore& mKVStore;
};
KVStore::Transaction KVStore::getTransaction()
{
- Lock lock(getTransactionMutex);
+ Lock lock(mMutex);
auto t = mTransactionImplPtr.lock();
if (!t) {
- t = std::make_shared(this);
+ t = std::make_shared(*this);
mTransactionImplPtr = t;
}
return t;
@@ -142,21 +144,14 @@ KVStore::KVStore(const std::string& path)
prepareStatements();
}
-KVStore::KVStore(const KVStore& store)
- : mPath(store.mPath),
- mConn(mPath)
-{
- createFunctions();
- prepareStatements();
-}
-
KVStore::~KVStore()
{
+ assert(!mTransactionImplPtr.lock());
}
void KVStore::setupDb()
{
- Transaction transaction = getTransaction();
+ // called only from ctor, transaction is not needed
mConn.exec("CREATE TABLE IF NOT EXISTS data (key TEXT PRIMARY KEY, value TEXT NOT NULL)");
}
diff --git a/src/config/kvstore.hpp b/src/config/kvstore.hpp
index 4dc978a..1b24b36 100644
--- a/src/config/kvstore.hpp
+++ b/src/config/kvstore.hpp
@@ -50,10 +50,12 @@ public:
/**
* @param path configuration database file path
*/
- KVStore(const std::string& path);
- KVStore(const KVStore& store);
+ explicit KVStore(const std::string& path);
~KVStore();
+ KVStore(const KVStore&) = delete;
+ KVStore& operator=(const KVStore&) = delete;
+
/**
* Clears all the stored data
*/
@@ -114,12 +116,11 @@ public:
KVStore::Transaction getTransaction();
private:
- typedef std::lock_guard Lock;
+ typedef std::lock_guard Lock;
struct TransactionImpl;
std::weak_ptr mTransactionImplPtr;
- std::mutex getTransactionMutex;
- std::mutex mConnMtx;
+ std::recursive_mutex mMutex;
void setInternal(const std::string& key, const std::string& value);
void setInternal(const std::string& key, const std::initializer_list& values);
--
2.7.4