[Datacontrol] Prevent possible race conditions

DataconstrolInstance::reply_map is accessed in callbacks
that can run in threads other than that, which runs API calls.
This commit adds a mutex and proper locks to prevent race
conditions.

[Verification] tct-datacontrol-tizen-tests: 100% pass rate

Change-Id: I2573a59c36af3e12a0d5e77598b170192d24f225
Signed-off-by: Pawel Wasowski <p.wasowski2@samsung.com>

diff --git a/src/datacontrol/datacontrol_instance.cc b/src/datacontrol/datacontrol_instance.cc
index a7694c5..d954fa7 100644 (file)
--- a/src/datacontrol/datacontrol_instance.cc
+++ b/src/datacontrol/datacontrol_instance.cc
@@ -109,6 +109,8 @@ DatacontrolInstance::DatacontrolInstance() {
 
 DatacontrolInstance::~DatacontrolInstance() {
   ScopeLogger();
+
+  std::lock_guard<std::mutex> lock{reply_map_mutex};
   for (auto& item : reply_map) {
     int watch_id = item.first;
     auto handle = item.second->handle;
@@ -1154,9 +1156,11 @@ void DatacontrolInstance::result_callback(data_control_h provider, data_control_
     // According to native documentation only IOError can be returned to webapi, other errors are
     // handled earlier
     LogAndReportError(IOException("DataControlConsumerObjectAddChangeListener failed"), obj);
+
     Instance::PostMessage(data->_instance, event.serialize().c_str());
     if (0 != watch_id) {
-      data->_instance->EraseMap(watch_id);
+      std::lock_guard<std::mutex> reply_map_lock{data->_instance->reply_map_mutex};
+      data->_instance->reply_map.erase(watch_id);
     }
   }
 }
@@ -1201,18 +1205,28 @@ void DatacontrolInstance::DataControlConsumerObjectAddChangeListener(const picoj
   user_data->handle = handle;
 
   int watch_id = 0;
-  result = ::data_control_add_data_change_cb(handle, callback, user_data.get(), result_callback,
-                                             user_data.get(), &watch_id);
+  {
+    /*
+     * "result_callback", which manipulates reply_map, can be called before or
+     * simultaneously with insertion of the watch_id to reply_map, which happens
+     * after checking result code.
+     * To prevent a race condition in access to reply_map, we lock its mutex before calling
+     * data_control_add_data_change_cb.
+     */
+    std::lock_guard<std::mutex> lock{reply_map_mutex};
+    result = ::data_control_add_data_change_cb(handle, callback, user_data.get(), result_callback,
+                                               user_data.get(), &watch_id);
 
-  if (DATA_CONTROL_ERROR_NONE != result) {
-    LogAndReportError(
-        ServiceNotAvailableException("DataControlConsumerObjectAddChangeListener failed"), out,
-        ("DataControlConsumerObjectAddChangeListener failed: %d (%s)", result,
-         get_error_message(result)));
-    return;
-  }
+    if (DATA_CONTROL_ERROR_NONE != result) {
+      LogAndReportError(
+          ServiceNotAvailableException("DataControlConsumerObjectAddChangeListener failed"), out,
+          ("DataControlConsumerObjectAddChangeListener failed: %d (%s)", result,
+           get_error_message(result)));
+      return;
+    }
 
-  reply_map.insert(std::pair<int, ReplyCallbackDataPtr>(watch_id, user_data));
+    reply_map.insert(std::pair<int, ReplyCallbackDataPtr>(watch_id, user_data));
+  }
 
   picojson::value return_value = picojson::value(picojson::object());
   picojson::object& return_value_obj = return_value.get<picojson::object>();
@@ -1231,6 +1245,8 @@ void DatacontrolInstance::DataControlConsumerObjectRemoveChangeListener(const pi
   CHECK_EXIST(args, "watchId", out)
 
   int watch_id = static_cast<int>(args.get("watchId").get<double>());
+
+  std::lock_guard<std::mutex> lock{reply_map_mutex};
   if (reply_map.end() == reply_map.find(watch_id)) {
     LogAndReportError(
         IOException("DataControlConsumerObjectRemoveChangeListener failed"), out,

diff --git a/src/datacontrol/datacontrol_instance.h b/src/datacontrol/datacontrol_instance.h
index cb639f0..8ad514a 100644 (file)
--- a/src/datacontrol/datacontrol_instance.h
+++ b/src/datacontrol/datacontrol_instance.h
@@ -57,11 +57,6 @@ class DatacontrolInstance : public common::ParsedInstance {
 
   int CreateSQLHandle(const std::string& providerId, const std::string& dataId,
                       data_control_h* handle);
-
-  void EraseMap(int watch_id) {
-    reply_map.erase(watch_id);
-  };
-
   static void callback(data_control_h provider, data_control_data_change_type_e type,
                        bundle* bundle_data, void* user_data);
   static void result_callback(data_control_h provider, data_control_error_e result, int callback_id,
@@ -83,6 +78,12 @@ class DatacontrolInstance : public common::ParsedInstance {
   void DataControlConsumerObjectRemoveChangeListener(const picojson::value& args,
                                                      picojson::object& out);
 
+  /*
+   * reply_map can be accessed in the thread, in which Web API calls are run
+   * and in callbacks which may run in in other threads. Thus we need a mutex to protect
+   * it from race conditions.
+   */
+  std::mutex reply_map_mutex;
   ReplyCallbackDataMap reply_map;
 };