From 1ba9c5d84259578bafb04e2f2be772c01747a5a9 Mon Sep 17 00:00:00 2001 From: Dan Mihai Date: Tue, 25 Oct 2016 11:33:58 -0700 Subject: [PATCH] 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 Reviewed-on: https://gerrit.iotivity.org/gerrit/13665 Tested-by: jenkins-iotivity Reviewed-by: Dave Thaler Reviewed-by: David Antler Reviewed-by: Ashok Babu Channa --- resource/c_common/iotivity_debug.h | 42 ++++++++++++++++++ resource/c_common/octhread/src/windows/octhread.c | 8 ++-- .../csdk/connectivity/src/ip_adapter/caipserver.c | 50 +++++++++++++--------- .../src/ip_adapter/windows/caipnwmonitor.c | 45 +++++++++---------- 4 files changed, 96 insertions(+), 49 deletions(-) create mode 100644 resource/c_common/iotivity_debug.h diff --git a/resource/c_common/iotivity_debug.h b/resource/c_common/iotivity_debug.h new file mode 100644 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 + +// 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 --- a/resource/c_common/octhread/src/windows/octhread.c +++ b/resource/c_common/octhread/src/windows/octhread.c @@ -32,6 +32,7 @@ #include #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 --- 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 #ifdef HAVE_SYS_SOCKET_H #include #endif -#include #ifdef HAVE_WINSOCK2_H #include #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 --- 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 @@ -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 } -- 2.7.4