IOT-1461 Fix double-close bugs

1. Fix two handle double-close bugs.
2. Add asserts to catch possible similar bugs.
3. Log WSAGetLastError in decimal, for easier match with the values
from MSDN docs.

Change-Id: I7f382344994c496c67e2e370d2241f080ce0e71c
Signed-off-by: Dan Mihai <Daniel.Mihai@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/13665
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Dave Thaler <dthaler@microsoft.com>
Reviewed-by: David Antler <david.a.antler@intel.com>
Reviewed-by: Ashok Babu Channa <ashok.channa@samsung.com>

diff --git a/resource/c_common/iotivity_debug.h b/resource/c_common/iotivity_debug.h
new file mode 100644 (file)
index 0000000..9a0141a
--- /dev/null
+++ b/resource/c_common/iotivity_debug.h
@@ -0,0 +1,42 @@
+//******************************************************************
+//
+// Copyright 2016 Microsoft
+//
+//-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+//******************************************************************
+
+/**
+ * @file
+ *
+ * This file contains debug helpers.
+ */
+
+#ifndef IOTIVITY_DEBUG_H_
+#define IOTIVITY_DEBUG_H_
+
+#include <assert.h>
+
+// Macro used to avoid the need for a local variable just for an assert. Using
+// a local variable just for assert, instead of this macro, can cause compiler
+// warnings on NDEBUG builds. Example: use OC_VERIFY(foo() == 0) instead of
+// {int local = foo(); assert(local == 0);}
+#if defined(NDEBUG)
+#define OC_VERIFY(condition) ((void)(condition))
+#else
+#define OC_VERIFY(condition) assert(condition)
+#endif
+
+#endif // #ifndef IOTIVITY_DEBUG_H_

diff --git a/resource/c_common/octhread/src/windows/octhread.c b/resource/c_common/octhread/src/windows/octhread.c
index 2613883..13259d8 100644 (file)
--- a/resource/c_common/octhread/src/windows/octhread.c
+++ b/resource/c_common/octhread/src/windows/octhread.c
@@ -32,6 +32,7 @@
 #include <oic_malloc.h>
 
 #include "logger.h"
+#include "iotivity_debug.h"
 
 static const uint64_t USECS_PER_MSEC = 1000;
 
@@ -85,7 +86,7 @@ OCThreadResult_t oc_thread_free(oc_thread t)
     oc_thread_internal *threadInfo = (oc_thread_internal*) t;
     if (threadInfo)
     {
-        CloseHandle(threadInfo->handle);
+        OC_VERIFY(CloseHandle(threadInfo->handle));
         OICFree(threadInfo);
         res = OC_THREAD_SUCCESS;
     }
@@ -101,15 +102,12 @@ OCThreadResult_t oc_thread_wait(oc_thread t)
     OCThreadResult_t res = OC_THREAD_SUCCESS;
     oc_thread_internal *threadInfo = (oc_thread_internal*) t;
     DWORD joinres = WaitForSingleObject(threadInfo->handle, INFINITE);
+    assert(WAIT_OBJECT_0 == joinres);
     if (WAIT_OBJECT_0 != joinres)
     {
         OIC_LOG(ERROR, TAG, "Failed to join thread");
         res = OC_THREAD_WAIT_FAILURE;
     }
-    else
-    {
-        CloseHandle(threadInfo->handle);
-    }
     return res;
 }
 

diff --git a/resource/csdk/connectivity/src/ip_adapter/caipserver.c b/resource/csdk/connectivity/src/ip_adapter/caipserver.c
index 3a70e93..f570802 100644 (file)
--- a/resource/csdk/connectivity/src/ip_adapter/caipserver.c
+++ b/resource/csdk/connectivity/src/ip_adapter/caipserver.c
@@ -26,11 +26,12 @@
 #endif
 
 #include "iotivity_config.h"
+#include "iotivity_debug.h"
+
 #include <sys/types.h>
 #ifdef HAVE_SYS_SOCKET_H
 #include <sys/socket.h>
 #endif
-#include <assert.h>
 #ifdef HAVE_WINSOCK2_H
 #include <winsock2.h>
 #endif
@@ -299,20 +300,21 @@ static void CASelectReturned(fd_set *readFds, int ret)
     INDEX++; \
 }
 
-// Turn handle into WSAEvent and push to ARRAY
+// Create WSAEvent for SOCKET and push the new event into ARRAY
 #define PUSH_SOCKET(SOCKET, ARRAY, INDEX) \
     if (SOCKET != OC_INVALID_SOCKET) \
     { \
-        WSAEVENT NewEvent; \
-        NewEvent = WSACreateEvent(); \
+        WSAEVENT NewEvent = WSACreateEvent(); \
         if (WSA_INVALID_EVENT != NewEvent) \
         { \
             if (0 != WSAEventSelect(SOCKET, NewEvent, FD_READ)) \
             { \
-                OIC_LOG_V(ERROR, TAG, "WSAEventSelect failed 0x%08x ", WSAGetLastError()); \
-                if (!WSACloseEvent(NewEvent)) \
+                OIC_LOG_V(ERROR, TAG, "WSAEventSelect failed %d", WSAGetLastError()); \
+                BOOL closed = WSACloseEvent(NewEvent); \
+                assert(closed); \
+                if (!closed) \
                 { \
-                    OIC_LOG_V(ERROR, TAG, "WSACloseEvent(NewEvent) failed 0x%08x", WSAGetLastError()); \
+                    OIC_LOG_V(ERROR, TAG, "WSACloseEvent(NewEvent) failed %d", WSAGetLastError()); \
                 } \
             } \
             else \
@@ -322,7 +324,7 @@ static void CASelectReturned(fd_set *readFds, int ret)
         } \
         else \
         { \
-            OIC_LOG_V(ERROR, TAG, "WSACreateEvent(NewEvent) failed 0x%08x", WSAGetLastError()); \
+            OIC_LOG_V(ERROR, TAG, "WSACreateEvent failed %d", WSAGetLastError()); \
         }\
     }
 
@@ -363,6 +365,7 @@ static void CAFindReadyMessage()
 
     // socketArray and eventArray should have same number of elements
     OC_STATIC_ASSERT(_countof(socketArray) == _countof(eventArray), "Arrays should have same number of elements");
+    OC_STATIC_ASSERT(_countof(eventArray) <= WSA_MAXIMUM_WAIT_EVENTS, "Too many events for a single Wait");
 
     PUSH_IP_SOCKET(u6,  eventArray, socketArray, arraySize);
     PUSH_IP_SOCKET(u6s, eventArray, socketArray, arraySize);
@@ -394,17 +397,19 @@ static void CAFindReadyMessage()
     while (!caglobals.ip.terminate)
     {
         int ret = WSAWaitForMultipleEvents(arraySize, eventArray, FALSE, WSA_INFINITE, FALSE);
+        assert(ret >= WSA_WAIT_EVENT_0);
+        assert(ret < (WSA_WAIT_EVENT_0 + arraySize));
 
         switch (ret)
         {
             case WSA_WAIT_FAILED:
-                OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents returned WSA_WAIT_FAILED 0x%08x", WSAGetLastError());
+                OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents returned WSA_WAIT_FAILED %d", WSAGetLastError());
                 break;
             case WSA_WAIT_IO_COMPLETION:
-                OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents returned WSA_WAIT_IO_COMPLETION 0x%08x", WSAGetLastError());
+                OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents returned WSA_WAIT_IO_COMPLETION %d", WSAGetLastError());
                 break;
             case WSA_WAIT_TIMEOUT:
-                OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents returned WSA_WAIT_TIMEOUT 0x%08x", WSAGetLastError());
+                OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents returned WSA_WAIT_TIMEOUT %d", WSAGetLastError());
                 break;
             default:
                 eventIndex = ret - WSA_WAIT_EVENT_0;
@@ -412,7 +417,7 @@ static void CAFindReadyMessage()
                 {
                     if (false == WSAResetEvent(eventArray[eventIndex]))
                     {
-                        OIC_LOG_V(ERROR, TAG, "WSAResetEvent failed 0x%08x", WSAGetLastError());
+                        OIC_LOG_V(ERROR, TAG, "WSAResetEvent failed %d", WSAGetLastError());
                     }
 
                     // Handle address changes if addressChangeEvent is triggered.
@@ -446,19 +451,24 @@ static void CAFindReadyMessage()
                 }
                 else
                 {
-                    OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents failed 0x%08x", WSAGetLastError());
+                    OIC_LOG_V(ERROR, TAG, "WSAWaitForMultipleEvents failed %d", WSAGetLastError());
                 }
                 break;
         }
 
     }
 
-    while (arraySize > 0)
+    for (int i = 0; i < arraySize; i++)
     {
-        arraySize--;
-        if (!WSACloseEvent(eventArray[arraySize]))
+        HANDLE h = eventArray[i];
+        if (h != caglobals.ip.addressChangeEvent)
         {
-            OIC_LOG_V(ERROR, TAG, "WSACloseEvent (Index %i) failed 0x%08x", arraySize, WSAGetLastError());
+            BOOL closed = WSACloseEvent(h);
+            assert(closed);
+            if (!closed)
+            {
+                OIC_LOG_V(ERROR, TAG, "WSACloseEvent (Index %i) failed %d", i, WSAGetLastError());
+            }
         }
     }
 
@@ -501,7 +511,7 @@ void CAUnregisterForAddressChanges()
 #ifdef _WIN32
     if (caglobals.ip.addressChangeEvent != WSA_INVALID_EVENT)
     {
-        WSACloseEvent(caglobals.ip.addressChangeEvent);
+        OC_VERIFY(WSACloseEvent(caglobals.ip.addressChangeEvent));
         caglobals.ip.addressChangeEvent = WSA_INVALID_EVENT;
     }
 #else
@@ -1018,7 +1028,7 @@ void CAIPStopServer()
     // receive thread will stop immediately.
     if (!WSASetEvent(caglobals.ip.shutdownEvent))
     {
-        OIC_LOG_V(DEBUG, TAG, "set shutdown event failed: %#08X", GetLastError());
+        OIC_LOG_V(DEBUG, TAG, "set shutdown event failed: %d", WSAGetLastError());
     }
 #endif
 }
@@ -1041,7 +1051,7 @@ void CAWakeUpForChange()
 #else
     if (!WSASetEvent(caglobals.ip.shutdownEvent))
     {
-        OIC_LOG_V(DEBUG, TAG, "set shutdown event failed: %#08X", GetLastError());
+        OIC_LOG_V(DEBUG, TAG, "set shutdown event failed: %d", WSAGetLastError());
     }
 #endif
 }

diff --git a/resource/csdk/connectivity/src/ip_adapter/windows/caipnwmonitor.c b/resource/csdk/connectivity/src/ip_adapter/windows/caipnwmonitor.c
index 2e401e2..e264fe7 100644 (file)
--- a/resource/csdk/connectivity/src/ip_adapter/windows/caipnwmonitor.c
+++ b/resource/csdk/connectivity/src/ip_adapter/windows/caipnwmonitor.c
@@ -18,6 +18,7 @@
 ******************************************************************/
 
 #include "iotivity_config.h"
+#include "iotivity_debug.h"
 #include "caipinterface.h"
 
 #include <assert.h>
@@ -157,7 +158,7 @@ static bool CACmpNetworkList(uint32_t ifIndex, int family, const char *addr, u_a
     return false;
 }
 
-static HANDLE g_CAIPNetworkMonitorChangeNotificationHandle = NULL;
+static HANDLE g_CAIPNetworkMonitorNotificationHandle = NULL;
 
 /**
  * Handle a notification that the IP address info changed.
@@ -223,11 +224,9 @@ static void CALLBACK IpAddressChangeCallback(void *context,
         // in order to join the multicast group on the associated interface and address family.
         if (g_CAIPNetworkMonitorNewAddressQueue)
         {
-            int ret = WSASetEvent(caglobals.ip.addressChangeEvent);
-
             // Setting the event should always succeed, since the handle should always be
             // valid when this code is reached.
-            assert(ret);
+            OC_VERIFY(WSASetEvent(caglobals.ip.addressChangeEvent));
         }
     }
 
@@ -246,18 +245,16 @@ void UnregisterForIpAddressChange()
     if (g_CAIPNetworkMonitorShutdownEvent != NULL)
     {
         // Cancel the worker thread.
-        if (SetEvent(g_CAIPNetworkMonitorShutdownEvent))
-        {
-            WaitForSingleObject(g_CAIPNetworkMonitorChangeNotificationHandle, INFINITE);
-        }
-
-        CloseHandle(g_CAIPNetworkMonitorShutdownEvent);
+        OC_VERIFY(SetEvent(g_CAIPNetworkMonitorShutdownEvent));
+        OC_VERIFY(WaitForSingleObject(g_CAIPNetworkMonitorNotificationHandle,
+                                      INFINITE) == WAIT_OBJECT_0);
+        OC_VERIFY(CloseHandle(g_CAIPNetworkMonitorShutdownEvent));
         g_CAIPNetworkMonitorShutdownEvent = NULL;
     }
-    if (g_CAIPNetworkMonitorChangeNotificationHandle != NULL)
+    if (g_CAIPNetworkMonitorNotificationHandle != NULL)
     {
-        CloseHandle(g_CAIPNetworkMonitorChangeNotificationHandle);
-        g_CAIPNetworkMonitorChangeNotificationHandle = NULL;
+        OC_VERIFY(CloseHandle(g_CAIPNetworkMonitorNotificationHandle));
+        g_CAIPNetworkMonitorNotificationHandle = NULL;
     }
 }
 
@@ -348,7 +345,7 @@ DWORD WINAPI IpNetworkMonitorWorker(PVOID context)
                     break;
                 }
 
-                WSAResetEvent(overlapped.hEvent);
+                OC_VERIFY(WSAResetEvent(overlapped.hEvent));
             }
         }
 
@@ -365,7 +362,7 @@ done:
     }
     if (overlapped.hEvent != NULL)
     {
-        CloseHandle(overlapped.hEvent);
+        OC_VERIFY(CloseHandle(overlapped.hEvent));
         overlapped.hEvent = NULL;
     }
     WSACleanup();
@@ -374,7 +371,7 @@ done:
 
 BOOL RegisterForIpAddressChange()
 {
-    assert(g_CAIPNetworkMonitorChangeNotificationHandle == NULL);
+    assert(g_CAIPNetworkMonitorNotificationHandle == NULL);
 
     g_CAIPNetworkMonitorShutdownEvent = CreateEvent(
         NULL, // No security descriptor.
@@ -386,7 +383,7 @@ BOOL RegisterForIpAddressChange()
         return false;
     }
 
-    g_CAIPNetworkMonitorChangeNotificationHandle = CreateThread(
+    g_CAIPNetworkMonitorNotificationHandle = CreateThread(
         NULL, // Default security attributes.
         0, // Default stack size.
         IpNetworkMonitorWorker,
@@ -394,9 +391,9 @@ BOOL RegisterForIpAddressChange()
         0, // Run immediately.
         NULL); // We don't need the thread id.
 
-    if (g_CAIPNetworkMonitorChangeNotificationHandle == NULL)
+    if (g_CAIPNetworkMonitorNotificationHandle == NULL)
     {
-        CloseHandle(g_CAIPNetworkMonitorShutdownEvent);
+        OC_VERIFY(CloseHandle(g_CAIPNetworkMonitorShutdownEvent));
         g_CAIPNetworkMonitorShutdownEvent = NULL;
         return false;
     }
@@ -431,7 +428,7 @@ CAResult_t CAIPStartNetworkMonitor(CAIPAdapterStateChangeCallback callback,
         return res;
     }
 
-    if (g_CAIPNetworkMonitorChangeNotificationHandle == NULL)
+    if (g_CAIPNetworkMonitorNotificationHandle == NULL)
     {
 #ifdef USE_SOCKET_ADDRESS_CHANGE_EVENT
         if (!RegisterForIpAddressChange())
@@ -441,7 +438,7 @@ CAResult_t CAIPStartNetworkMonitor(CAIPAdapterStateChangeCallback callback,
 #else
         int err = NotifyUnicastIpAddressChange(AF_UNSPEC, IpAddressChangeCallback, NULL,
                                                true,
-                                               &g_CAIPNetworkMonitorChangeNotificationHandle);
+                                               &g_CAIPNetworkMonitorNotificationHandle);
         if (err != NO_ERROR)
         {
             return CA_STATUS_FAILED;
@@ -459,14 +456,14 @@ CAResult_t CAIPStartNetworkMonitor(CAIPAdapterStateChangeCallback callback,
  */
 CAResult_t CAIPStopNetworkMonitor(CATransportAdapter_t adapter)
 {
-    if (g_CAIPNetworkMonitorChangeNotificationHandle != NULL)
+    if (g_CAIPNetworkMonitorNotificationHandle != NULL)
     {
 #ifdef USE_SOCKET_ADDRESS_CHANGE_EVENT
         UnregisterForIpAddressChange();
 #else
-        int err = CancelMibChangeNotify2(g_CAIPNetworkMonitorChangeNotificationHandle);
+        int err = CancelMibChangeNotify2(g_CAIPNetworkMonitorNotificationHandle);
         assert(err == NO_ERROR);
-        g_CAIPNetworkMonitorChangeNotificationHandle = NULL;
+        g_CAIPNetworkMonitorNotificationHandle = NULL;
 #endif
     }