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

diff --git a/src/config/from-kvjson-visitor.hpp b/src/config/from-kvjson-visitor.hpp
index c355fc2..2e386b5 100644 (file)
--- 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 (file)
--- a/src/config/kvstore.cpp
+++ b/src/config/kvstore.cpp
@@ -31,6 +31,7 @@
 #include <limits>
 #include <memory>
 #include <set>
+#include <cassert>
 
 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<TransactionImpl>(this);
+        t = std::make_shared<TransactionImpl>(*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 (file)
--- 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<std::mutex> Lock;
+    typedef std::lock_guard<std::recursive_mutex> Lock;
 
     struct TransactionImpl;
     std::weak_ptr<TransactionImpl> 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<std::string>& values);