IOT-1302 Thread-unsafe code in oicgroup.c

Remove potential thread safety bugs in oicgroup
Make RD mutex implementation work on Windows
Remove duplicate mutex implementations

Change-Id: Ie47b24619a10ab3d9cab2d421f3a7d8be7ac7b55
Signed-off-by: Dave Thaler <dthaler@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/11941
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: David Antler <david.a.antler@intel.com>
Reviewed-by: Uze Choi <uzchoi@samsung.com>
Reviewed-by: Habib Virji <habib.virji@samsung.com>
Reviewed-by: jihwan seo <jihwan.seo@samsung.com>

diff --git a/resource/csdk/stack/include/internal/oicgroup.h b/resource/csdk/stack/include/internal/oicgroup.h
index 6ba6be2..0392704 100755 (executable)
--- a/resource/csdk/stack/include/internal/oicgroup.h
+++ b/resource/csdk/stack/include/internal/oicgroup.h
@@ -56,6 +56,9 @@ OCStackApplicationResult ActionSetCB(void* context, OCDoHandle handle,
 
 void ActionSetCD(void *context);
 
+OCStackResult InitializeScheduleResourceList();
+
+void TerminateScheduleResourceList();
 
 OCStackResult
 BuildCollectionGroupActionCBORResponse(OCMethod method/*OCEntityHandlerFlag flag*/,

diff --git a/resource/csdk/stack/src/ocstack.c b/resource/csdk/stack/src/ocstack.c
index 044604d..bad08d8 100644 (file)
--- a/resource/csdk/stack/src/ocstack.c
+++ b/resource/csdk/stack/src/ocstack.c
@@ -59,6 +59,7 @@
 #include "ocpayload.h"
 #include "ocpayloadcbor.h"
 #include "cautilinterface.h"
+#include "oicgroup.h"
 
 #if defined (ROUTING_GATEWAY) || defined (ROUTING_EP)
 #include "routingutility.h"
@@ -2243,6 +2244,9 @@ OCStackResult OCInit1(OCMode mode, OCTransportFlags serverFlags, OCTransportFlag
     defaultDeviceHandler = NULL;
     defaultDeviceHandlerCallbackParameter = NULL;
 
+    result = InitializeScheduleResourceList();
+    VERIFY_SUCCESS(result, OC_STACK_OK);
+
     result = CAResultToOCResult(CAInitialize());
     VERIFY_SUCCESS(result, OC_STACK_OK);
 
@@ -2323,6 +2327,7 @@ exit:
         OIC_LOG(ERROR, TAG, "Stack initialization error");
         deleteAllResources();
         CATerminate();
+        TerminateScheduleResourceList();
         stackState = OC_STACK_UNINITIALIZED;
     }
     return result;
@@ -2375,6 +2380,7 @@ OCStackResult OCStop()
     DeleteDeviceInfo();
     DeletePlatformInfo();
     CATerminate();
+    TerminateScheduleResourceList();
     // Remove all observers
     DeleteObserverList();
     // Remove all the client callbacks

diff --git a/resource/csdk/stack/src/oicgroup.c b/resource/csdk/stack/src/oicgroup.c
index 8a9e443..f0c0d4a 100755 (executable)
--- a/resource/csdk/stack/src/oicgroup.c
+++ b/resource/csdk/stack/src/oicgroup.c
@@ -30,6 +30,7 @@
 #include "ocpayload.h"
 #include "oic_malloc.h"
 #include "oic_string.h"
+#include "octhread.h"
 #include "occollection.h"
 #include "logger.h"
 #include "timer.h"
@@ -69,31 +70,7 @@
         pointer = NULL; \
     }
 
-// Mutex implementation macros
-#if defined(HAVE_PTHREAD_H)
-
- #include <pthread.h>
- pthread_mutex_t lock;
- #define MUTEX_LOCK(ARG_NAME)    { pthread_mutex_lock(ARG_NAME); }
- #define MUTEX_UNLOCK(ARG_NAME)  { pthread_mutex_unlock(ARG_NAME); }
-
-#elif defined(HAVE_WINDOWS_H)
-
- #include <windows.h>
- CRITICAL_SECTION lock;
- #define MUTEX_LOCK(ARG_NAME)   { EnterCriticalSection(ARG_NAME); }
- #define MUTEX_UNLOCK(ARG_NAME) { LeaveCriticalSection(ARG_NAME); }
-
-#elif defined(WITH_ARDUINO)
-
- #define MUTEX_LOCK(ARG_NAME)   {  }
- #define MUTEX_UNLOCK(ARG_NAME) {  }
-
-#else
-
- ERROR Need mutex implementation on this platform
-
-#endif
+oc_mutex g_scheduledResourceLock = NULL;
 
 enum ACTION_TYPE
 {
@@ -113,14 +90,14 @@ typedef struct scheduledresourceinfo
     struct scheduledresourceinfo* next;
 } ScheduledResourceInfo;
 
-ScheduledResourceInfo *scheduleResourceList = NULL;
+ScheduledResourceInfo *g_scheduleResourceList = NULL;
 
 void AddScheduledResource(ScheduledResourceInfo **head,
         ScheduledResourceInfo* add)
 {
     OIC_LOG(INFO, TAG, "AddScheduledResource Entering...");
 
-    MUTEX_LOCK(&lock);
+    oc_mutex_lock(g_scheduledResourceLock);
     ScheduledResourceInfo *tmp = NULL;
 
     if (*head != NULL)
@@ -137,14 +114,14 @@ void AddScheduledResource(ScheduledResourceInfo **head,
     {
         *head = add;
     }
-    MUTEX_UNLOCK(&lock);
+    oc_mutex_unlock(g_scheduledResourceLock);
 }
 
 ScheduledResourceInfo* GetScheduledResource(ScheduledResourceInfo *head)
 {
     OIC_LOG(INFO, TAG, "GetScheduledResource Entering...");
 
-    MUTEX_LOCK(&lock);
+    oc_mutex_lock(g_scheduledResourceLock);
 
     time_t t_now;
 
@@ -176,7 +153,7 @@ ScheduledResourceInfo* GetScheduledResource(ScheduledResourceInfo *head)
 
     exit:
 
-    MUTEX_UNLOCK(&lock);
+    oc_mutex_unlock(g_scheduledResourceLock);
 
     if (tmp == NULL)
     {
@@ -189,7 +166,7 @@ ScheduledResourceInfo* GetScheduledResourceByActionSetName(ScheduledResourceInfo
 {
     OIC_LOG(INFO, TAG, "GetScheduledResourceByActionSetName Entering...");
 
-    MUTEX_LOCK(&lock);
+    oc_mutex_lock(g_scheduledResourceLock);
 
     ScheduledResourceInfo *tmp = NULL;
     tmp = head;
@@ -209,7 +186,7 @@ ScheduledResourceInfo* GetScheduledResourceByActionSetName(ScheduledResourceInfo
 
 exit:
 
-    MUTEX_UNLOCK(&lock);
+    oc_mutex_unlock(g_scheduledResourceLock);
 
     if (tmp == NULL)
     {
@@ -222,7 +199,7 @@ void RemoveScheduledResource(ScheduledResourceInfo **head,
         ScheduledResourceInfo* del)
 {
 
-    MUTEX_LOCK(&lock);
+    oc_mutex_lock(g_scheduledResourceLock);
 
     OIC_LOG(INFO, TAG, "RemoveScheduledResource Entering...");
     ScheduledResourceInfo *tmp = NULL;
@@ -230,7 +207,7 @@ void RemoveScheduledResource(ScheduledResourceInfo **head,
     if (del == NULL)
     {
 
-        MUTEX_UNLOCK(&lock);
+        oc_mutex_unlock(g_scheduledResourceLock);
 
         return;
     }
@@ -254,8 +231,7 @@ void RemoveScheduledResource(ScheduledResourceInfo **head,
 
     OCFREE(del)
 
-    MUTEX_UNLOCK(&lock);
-
+    oc_mutex_unlock(g_scheduledResourceLock);
 }
 
 typedef struct aggregatehandleinfo
@@ -1085,7 +1061,7 @@ OCStackResult DoAction(OCResource* resource, OCActionSet* actionset,
 void DoScheduledGroupAction()
 {
     OIC_LOG(INFO, TAG, "DoScheduledGroupAction Entering...");
-    ScheduledResourceInfo* info = GetScheduledResource(scheduleResourceList);
+    ScheduledResourceInfo* info = GetScheduledResource(g_scheduleResourceList);
 
     if (info == NULL)
     {
@@ -1108,11 +1084,11 @@ void DoScheduledGroupAction()
         goto exit;
     }
 
-    MUTEX_LOCK(&lock);
+    oc_mutex_lock(g_scheduledResourceLock);
 
     DoAction(info->resource, info->actionset, info->ehRequest);
 
-    MUTEX_UNLOCK(&lock);
+    oc_mutex_unlock(g_scheduledResourceLock);
 
 
     if (info->actionset->type == RECURSIVE)
@@ -1128,7 +1104,7 @@ void DoScheduledGroupAction()
 
             if (info->actionset->timesteps > 0)
             {
-                MUTEX_LOCK(&lock);
+                oc_mutex_lock(g_scheduledResourceLock);
                 schedule->resource = info->resource;
                 schedule->actionset = info->actionset;
                 schedule->ehRequest = info->ehRequest;
@@ -1137,9 +1113,9 @@ void DoScheduledGroupAction()
                         &schedule->timer_id,
                         &DoScheduledGroupAction);
 
-                OIC_LOG(INFO, TAG, "Reregisteration.");
-                MUTEX_UNLOCK(&lock);
-                AddScheduledResource(&scheduleResourceList, schedule);
+                OIC_LOG(INFO, TAG, "Reregistration.");
+                oc_mutex_unlock(g_scheduledResourceLock);
+                AddScheduledResource(&g_scheduleResourceList, schedule);
             }
             else
             {
@@ -1148,7 +1124,7 @@ void DoScheduledGroupAction()
         }
     }
 
-    RemoveScheduledResource(&scheduleResourceList, info);
+    RemoveScheduledResource(&g_scheduleResourceList, info);
 
     exit:
 
@@ -1166,15 +1142,7 @@ OCStackResult BuildCollectionGroupActionCBORResponse(
     char *doWhat = NULL;
     char *details = NULL;
 
-#if defined(_WIN32)
-    static bool initializedCriticalSection = false;
-
-    if(false == initializedCriticalSection) {
-        /** @todo Find a way to DeleteCriticalSection somewhere. */
-        InitializeCriticalSection(&lock);
-        initializedCriticalSection = true;
-    }
-#endif
+    assert(g_scheduledResourceLock != NULL);
 
     stackRet = ExtractKeyValueFromRequest(ehRequest, &doWhat, &details);
 
@@ -1346,22 +1314,22 @@ OCStackResult BuildCollectionGroupActionCBORResponse(
                             OIC_LOG(INFO, TAG, "Building New Call Info.");
                             memset(schedule, 0,
                                     sizeof(ScheduledResourceInfo));
-                            MUTEX_LOCK(&lock);
+                            oc_mutex_lock(g_scheduledResourceLock);
                             schedule->resource = resource;
                             schedule->actionset = actionset;
                             schedule->ehRequest =
                                     (OCServerRequest*) ehRequest->requestHandle;
-                            MUTEX_UNLOCK(&lock);
+                            oc_mutex_unlock(g_scheduledResourceLock);
                             if (delay > 0)
                             {
                                 OIC_LOG_V(INFO, TAG, "delay_time is %ld seconds.",
                                         actionset->timesteps);
-                                MUTEX_LOCK(&lock);
+                                oc_mutex_lock(g_scheduledResourceLock);
                                 schedule->time = registerTimer(delay,
                                         &schedule->timer_id,
                                         &DoScheduledGroupAction);
-                                MUTEX_UNLOCK(&lock);
-                                AddScheduledResource(&scheduleResourceList,
+                                oc_mutex_unlock(g_scheduledResourceLock);
+                                AddScheduledResource(&g_scheduleResourceList,
                                         schedule);
                                 stackRet = OC_STACK_OK;
                             }
@@ -1377,13 +1345,15 @@ OCStackResult BuildCollectionGroupActionCBORResponse(
         else if (strcmp(doWhat, "CancelAction") == 0)
         {
             ScheduledResourceInfo *info =
-                    GetScheduledResourceByActionSetName(scheduleResourceList, details);
+                    GetScheduledResourceByActionSetName(g_scheduleResourceList, details);
 
             if(info != NULL)
             {
+                oc_mutex_lock(g_scheduledResourceLock);
                 unregisterTimer(info->timer_id);
+                oc_mutex_unlock(g_scheduledResourceLock);
 
-                RemoveScheduledResource(&scheduleResourceList, info);
+                RemoveScheduledResource(&g_scheduleResourceList, info);
                 stackRet = OC_STACK_OK;
             }
             else
@@ -1452,3 +1422,28 @@ exit:
 
     return stackRet;
 }
+
+OCStackResult InitializeScheduleResourceList()
+{
+    assert(g_scheduledResourceLock == NULL);
+
+    g_scheduledResourceLock = oc_mutex_new();
+    if (g_scheduledResourceLock == NULL)
+    {
+        return OC_STACK_ERROR;
+    }
+
+    g_scheduleResourceList = NULL;
+    return OC_STACK_OK;
+}
+
+void TerminateScheduleResourceList()
+{
+    assert(g_scheduleResourceList == NULL);
+
+    if (g_scheduledResourceLock != NULL)
+    {
+        oc_mutex_free(g_scheduledResourceLock);
+        g_scheduledResourceLock = NULL;
+    }
+}

diff --git a/service/resource-directory/src/internal/rd_storage.c b/service/resource-directory/src/internal/rd_storage.c
index c782560..8c4eec1 100644 (file)
--- a/service/resource-directory/src/internal/rd_storage.c
+++ b/service/resource-directory/src/internal/rd_storage.c
@@ -18,13 +18,6 @@
 //
 //-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 #include "rd_storage.h"
-
-#ifdef HAVE_PTHREAD_H
-#include <pthread.h>
-#endif
-#ifdef HAVE_WINDOWS_H
-#include <windows.h>
-#endif
 #include <string.h>
 
 #include "payload_logging.h"
@@ -35,28 +28,7 @@
 
 #define TAG  PCF("RDStorage")
 
-// Mutex implementation macros
-#if defined(HAVE_PTHREAD_H)
-
-#include <pthread.h>
-pthread_mutex_t storageMutex;
-#define MUTEX_LOCK(ARG_NAME)    { pthread_mutex_lock(ARG_NAME); }
-#define MUTEX_UNLOCK(ARG_NAME)  { pthread_mutex_unlock(ARG_NAME); }
-
-#elif defined(HAVE_WINDOWS_H)
-
-#include <windows.h>
-CRITICAL_SECTION storageMutex;
-#define MUTEX_LOCK(ARG_NAME)   { EnterCriticalSection(ARG_NAME); }
-#define MUTEX_UNLOCK(ARG_NAME) { LeaveCriticalSection(ARG_NAME); }
-
-#else
-
-ERROR Need mutex implementation for this platform
-#define MUTEX_LOCK(ARG_NAME)   {  }
-#define MUTEX_UNLOCK(ARG_NAME) {  }
-
-#endif
+oc_mutex g_storageMutex = NULL;
 
 
 static OCRDStorePublishResources *g_rdStorage = NULL;
@@ -136,7 +108,7 @@ OCStackResult OCRDStorePublishedResources(const OCResourceCollectionPayload *pay
     resources->publishedResource = storeResource;
     resources->devAddr = *address;
 
-    MUTEX_LOCK(&storageMutex);
+    oc_mutex_lock(g_storageMutex);
     if (g_rdStorage)
     {
         OCRDStorePublishResources *temp = g_rdStorage;
@@ -150,7 +122,7 @@ OCStackResult OCRDStorePublishedResources(const OCResourceCollectionPayload *pay
     {
         g_rdStorage = resources;
     }
-    MUTEX_UNLOCK(&storageMutex);
+    oc_mutex_unlock(g_storageMutex);
 
     printStoragedResources(g_rdStorage);
     return OC_STACK_OK;
@@ -259,3 +231,36 @@ OCStackResult OCRDCheckPublishedResource(const char *interfaceType, const char *
     }
     return OC_STACK_ERROR;
 }
+
+/**
+ * Initializes the publish resources.
+ *
+ * @return ::OC_STACK_OK upon success, ::OC_STACK_ERROR in case of error.
+ */
+OCStackResult OCRDInitializeStorage()
+{
+    assert(g_storageMutex == NULL);
+
+    g_storageMutex = oc_mutex_new();
+    if (g_storageMutex == NULL)
+    {
+        return OC_STACK_ERROR;
+    }
+
+    g_rdStorage = NULL;
+    return OC_STACK_OK;
+}
+
+/**
+ * Cleans up the publish resources.
+ */
+void OCRDTerminateStorage()
+{
+    assert(g_rdStorage == NULL);
+
+    if (g_storageMutex != NULL)
+    {
+        oc_mutex_free(g_storageMutex);
+        g_storageMutex = NULL;
+    }
+}

diff --git a/service/resource-directory/src/internal/rd_storage.h b/service/resource-directory/src/internal/rd_storage.h
index e91cee9..f60c791 100644 (file)
--- a/service/resource-directory/src/internal/rd_storage.h
+++ b/service/resource-directory/src/internal/rd_storage.h
@@ -46,6 +46,18 @@ typedef struct OCRDStorePublishResources
  */
 OCStackResult OCRDStorePublishedResources(const OCResourceCollectionPayload *payload, const OCDevAddr *address);
 
+/**
+ * Initializes the publish resources.
+ *
+ * @return ::OC_STACK_OK upon success, ::OC_STACK_ERROR in case of error.
+ */
+OCStackResult OCRDInitializeStorage();
+
+/**
+ * Cleans up the publish resources.
+ */
+void OCRDTerminateStorage();
+
 #ifdef __cplusplus
 }
 #endif // __cplusplus

diff --git a/service/resource-directory/src/rd_server.c b/service/resource-directory/src/rd_server.c
index c9c72c2..7664c64 100644 (file)
--- a/service/resource-directory/src/rd_server.c
+++ b/service/resource-directory/src/rd_server.c
@@ -164,20 +164,24 @@ OCStackResult OCRDStart()
 {
     OCResourceHandle rdHandle = NULL;
 
-    OCStackResult result = OCCreateResource(&rdHandle,
-                                OC_RSRVD_RESOURCE_TYPE_RD,
-                                OC_RSRVD_INTERFACE_DEFAULT,
-                                OC_RSRVD_RD_URI,
-                                rdEntityHandler,
-                                NULL,
-                                (OC_ACTIVE | OC_DISCOVERABLE | OC_OBSERVABLE));
-
+    OCStackResult result = OCRDInitializeStorage();
+    if (result == OC_STACK_OK)
+    {
+        result = OCCreateResource(&rdHandle,
+                                  OC_RSRVD_RESOURCE_TYPE_RD,
+                                  OC_RSRVD_INTERFACE_DEFAULT,
+                                  OC_RSRVD_RD_URI,
+                                  rdEntityHandler,
+                                  NULL,
+                                  (OC_ACTIVE | OC_DISCOVERABLE | OC_OBSERVABLE));
+    }
     if (result == OC_STACK_OK)
     {
         OIC_LOG(DEBUG, TAG, "Resource Directory Started.");
     }
     else
     {
+        OCRDTerminateStorage();
         OIC_LOG(ERROR, TAG, "Failed starting Resource Directory.");
     }
 
@@ -189,6 +193,8 @@ OCStackResult OCRDStart()
  */
 OCStackResult OCRDStop()
 {
+    OCRDTerminateStorage();
+
     OCStackResult result = OCStop();
 
     if (result == OC_STACK_OK)