From 296711bf4683301528220e14be0356c78089afb6 Mon Sep 17 00:00:00 2001 From: KIM JungYong Date: Mon, 17 Apr 2017 20:41:03 +0900 Subject: [PATCH] Fix result of notification valgrind memory checker. When valgrind of notification service(including cpp wrapper) is run, several leak was detected. (memory leak and invalid use of memory) in this patch, result of valgrind was fixed. 1. invalid use of string in the unit test code, was fixed. 2. when returnning with error, allocated memory was freed. 3. unfreed variable was freed. 4. unit test build warning was fixed. Change-Id: I4c8367bfda86813bb1ce1c71cf7841067c489ccc Signed-off-by: KIM JungYong Reviewed-on: https://gerrit.iotivity.org/gerrit/19009 Tested-by: jenkins-iotivity Reviewed-by: Uze Choi --- .../cpp-wrapper/provider/src/NSConsumer.cpp | 21 +++++++++++++++++++++ .../cpp-wrapper/provider/src/NSProviderService.cpp | 21 +++++++++++++++++++++ .../cpp-wrapper/unittest/NSConsumerServiceTest2.cpp | 16 +++++++++++++--- .../cpp-wrapper/unittest/NSProviderServiceTest2.cpp | 20 ++++++++++---------- .../notification/src/consumer/NSConsumerDiscovery.c | 3 ++- .../notification/src/provider/NSProviderListener.c | 3 +++ .../src/provider/NSProviderSubscription.c | 2 ++ service/notification/src/provider/NSProviderTopic.c | 5 +++++ service/notification/unittest/NSConsumerTest2.cpp | 12 +++++++----- service/notification/unittest/NSProviderTest2.cpp | 12 ++++++++++-- 10 files changed, 94 insertions(+), 21 deletions(-) diff --git a/service/notification/cpp-wrapper/provider/src/NSConsumer.cpp b/service/notification/cpp-wrapper/provider/src/NSConsumer.cpp index 5d3c472..1ddca55 100755 --- a/service/notification/cpp-wrapper/provider/src/NSConsumer.cpp +++ b/service/notification/cpp-wrapper/provider/src/NSConsumer.cpp @@ -32,6 +32,26 @@ namespace OIC { namespace Service { + namespace + { + void removeTopicLL(NSTopicLL * topicHead) + { + NSTopicLL * iter = topicHead; + NSTopicLL * following = NULL; + + while (iter) + { + following = iter->next; + + OICFree(iter->topicName); + iter->next = NULL; + OICFree(iter); + + iter = following; + } + } + } + ::NSConsumer *NSConsumer::getNSConsumer() { ::NSConsumer *nsCon = new ::NSConsumer; @@ -100,6 +120,7 @@ namespace OIC ::NSTopicLL *topics = NSProviderGetConsumerTopics(getConsumerId().c_str()); std::shared_ptr nsTopics = std::make_shared(topics, false); + removeTopicLL(topics); NS_LOG(DEBUG, "getConsumerTopicList - OUT"); return nsTopics; } diff --git a/service/notification/cpp-wrapper/provider/src/NSProviderService.cpp b/service/notification/cpp-wrapper/provider/src/NSProviderService.cpp index c6332cc..510b2cd 100755 --- a/service/notification/cpp-wrapper/provider/src/NSProviderService.cpp +++ b/service/notification/cpp-wrapper/provider/src/NSProviderService.cpp @@ -36,6 +36,26 @@ namespace OIC { namespace Service { + namespace + { + void removeTopicLL(NSTopicLL * topicHead) + { + NSTopicLL * iter = topicHead; + NSTopicLL * following = NULL; + + while (iter) + { + following = iter->next; + + OICFree(iter->topicName); + iter->next = NULL; + OICFree(iter); + + iter = following; + } + } + } + void NSProviderService::onConsumerSubscribedCallback(::NSConsumer *consumer) { NS_LOG(DEBUG, "onConsumerSubscribedCallback - IN"); @@ -264,6 +284,7 @@ namespace OIC std::shared_ptr nsTopics = std::make_shared(topics, false); NS_LOG(DEBUG, "getRegisteredTopicList - OUT"); + removeTopicLL(topics); return nsTopics; } diff --git a/service/notification/cpp-wrapper/unittest/NSConsumerServiceTest2.cpp b/service/notification/cpp-wrapper/unittest/NSConsumerServiceTest2.cpp index 3bec0be..fb79774 100644 --- a/service/notification/cpp-wrapper/unittest/NSConsumerServiceTest2.cpp +++ b/service/notification/cpp-wrapper/unittest/NSConsumerServiceTest2.cpp @@ -216,7 +216,7 @@ TEST_F(NotificationServiceConsumerTest, ExpectGetProviderSuccessWithInvalidProvi TEST_F(NotificationServiceConsumerTest, ExpectGetProviderSuccessWithValidProviderId) { ::NSProvider *provider = (::NSProvider *)malloc(sizeof(::NSProvider)); - strcpy(provider->providerId, "test"); + strcpy(provider->providerId, "098765432109876543210987654321098765"); std::string provId; provId.assign(provider->providerId, NS_UTILS_UUID_STRING_SIZE - 1); @@ -242,7 +242,7 @@ TEST_F(NotificationServiceConsumerTest, ExpectSuccessSendSyncInfo) std::string provId; ::NSProvider *provider = (::NSProvider *)malloc(sizeof(::NSProvider)); - strcpy(provider->providerId, "test"); + strcpy(provider->providerId, "098765432109876543210987654321098765"); provId.assign(provider->providerId, NS_UTILS_UUID_STRING_SIZE - 1); std::shared_ptr providerTemp = std::make_shared @@ -260,6 +260,10 @@ TEST_F(NotificationServiceConsumerTest, ExpectSuccessSendSyncInfo) res = resProvider->sendSyncInfo(msgId, OIC::Service::NSSyncInfo::NSSyncType::NS_SYNC_READ); } EXPECT_EQ(OIC::Service::NSResult::OK, res); + + OIC::Service::NSConsumerService::getInstance() + ->getAcceptedProviders()->removeProvider(provider->providerId); + free(provider); } TEST_F(NotificationServiceConsumerTest, ExpectSuccessGetTopicsList) @@ -267,7 +271,7 @@ TEST_F(NotificationServiceConsumerTest, ExpectSuccessGetTopicsList) std::string provId; ::NSProvider *provider = (::NSProvider *)malloc(sizeof(::NSProvider)); - strcpy(provider->providerId, "test"); + strcpy(provider->providerId, "098765432109876543210987654321098765"); provId.assign(provider->providerId, NS_UTILS_UUID_STRING_SIZE - 1); std::shared_ptr providerTemp = std::make_shared @@ -283,5 +287,11 @@ TEST_F(NotificationServiceConsumerTest, ExpectSuccessGetTopicsList) auto topicList = resProvider->getTopicList(); ASSERT_NE(nullptr, topicList) << "Get topics list failure"; + + OIC::Service::NSConsumerService::getInstance() + ->getAcceptedProviders()->removeProvider(provider->providerId); + free(provider); + + OIC::Service::NSConsumerService::getInstance()->stop(); } diff --git a/service/notification/cpp-wrapper/unittest/NSProviderServiceTest2.cpp b/service/notification/cpp-wrapper/unittest/NSProviderServiceTest2.cpp index 363990a..c045c4d 100644 --- a/service/notification/cpp-wrapper/unittest/NSProviderServiceTest2.cpp +++ b/service/notification/cpp-wrapper/unittest/NSProviderServiceTest2.cpp @@ -66,23 +66,23 @@ class NotificationProviderServiceTest : public TestWithMock NotificationProviderServiceTest() = default; ~NotificationProviderServiceTest() = default; - static void ConsumerSubscribedCallback(std::shared_ptr consumer) + static void ConsumerSubscribedCallback(std::shared_ptr ) { std::cout << __func__ << std::endl; } - static void MessageSynchronizedCallback(OIC::Service::NSSyncInfo sync) + static void MessageSynchronizedCallback(OIC::Service::NSSyncInfo ) { std::cout << __func__ << std::endl; } static void MessageCallbackFromConsumer( - const int &id, const std::string &, const std::string &, const std::string &) + const int &, const std::string &, const std::string &, const std::string &) { std::cout << __func__ << std::endl; } - static void SyncCallbackFromConsumer(const int type, const int syncId) + static void SyncCallbackFromConsumer(const int , const int ) { std::cout << __func__ << std::endl; } @@ -236,7 +236,7 @@ TEST_F(NotificationProviderServiceTest, ExpectFailAcceptSubscriptionInvalidPolic OIC::Service::NSProviderService::getInstance()->start(config); ::NSConsumer *consumer = (::NSConsumer *)malloc(sizeof(::NSConsumer)); - strcpy(consumer->consumerId, "test"); + strcpy(consumer->consumerId, "098765432109876543210987654321098765"); std::shared_ptr consumerTemp = std::make_shared (consumer); @@ -256,7 +256,7 @@ TEST_F(NotificationProviderServiceTest, ExpectFailSetTopicInvalidPolicy) std::string str1("TEST1"); OIC::Service::NSProviderService::getInstance()->registerTopic(str1); ::NSConsumer *consumer = (::NSConsumer *)malloc(sizeof(::NSConsumer)); - strcpy(consumer->consumerId, "test"); + strcpy(consumer->consumerId, "098765432109876543210987654321098765"); std::shared_ptr consumerTemp = std::make_shared (consumer); @@ -282,7 +282,7 @@ TEST_F(NotificationProviderServiceTest, ExpectSuccessAcceptSubscription) OIC::Service::NSProviderService::getInstance()->start(config); ::NSConsumer *consumer = (::NSConsumer *)malloc(sizeof(::NSConsumer)); - strcpy(consumer->consumerId, "test"); + strcpy(consumer->consumerId, "098765432109876543210987654321098765"); std::shared_ptr consumerTemp = std::make_shared (consumer); @@ -303,7 +303,7 @@ TEST_F(NotificationProviderServiceTest, ExpectSuccessSetTopic) OIC::Service::NSProviderService::getInstance()->registerTopic(str1); ::NSConsumer *consumer = (::NSConsumer *)malloc(sizeof(::NSConsumer)); - strcpy(consumer->consumerId, "test"); + strcpy(consumer->consumerId, "098765432109876543210987654321098765"); std::shared_ptr consumerTemp = std::make_shared (consumer); @@ -321,7 +321,7 @@ TEST_F(NotificationProviderServiceTest, ExpectSuccessSetTopic) TEST_F(NotificationProviderServiceTest, ExpectSuccessUnSetTopic) { ::NSConsumer *consumer = (::NSConsumer *)malloc(sizeof(::NSConsumer)); - strcpy(consumer->consumerId, "test"); + strcpy(consumer->consumerId, "098765432109876543210987654321098765"); std::shared_ptr consumerTemp = std::make_shared (consumer); @@ -376,7 +376,7 @@ TEST_F(NotificationProviderServiceTest, ExpectEqualSetConsumerTopicsAndGetConsum std::string str1("TEST1"); std::string str2("TEST2"); ::NSConsumer *consumer = (::NSConsumer *)malloc(sizeof(::NSConsumer)); - strcpy(consumer->consumerId, "test"); + strcpy(consumer->consumerId, "098765432109876543210987654321098765"); std::shared_ptr consumerTemp = std::make_shared (consumer); diff --git a/service/notification/src/consumer/NSConsumerDiscovery.c b/service/notification/src/consumer/NSConsumerDiscovery.c index 28c137e..3fea1fb 100644 --- a/service/notification/src/consumer/NSConsumerDiscovery.c +++ b/service/notification/src/consumer/NSConsumerDiscovery.c @@ -68,7 +68,7 @@ OCStackApplicationResult NSConsumerPresenceListener( memcpy(addr, clientResponse->addr, sizeof(OCDevAddr)); NSTask * task = NSMakeTask(TASK_CONSUMER_PROVIDER_DELETED, addr); - NS_VERIFY_NOT_NULL(task, OC_STACK_KEEP_TRANSACTION); + NS_VERIFY_NOT_NULL_WITH_POST_CLEANING(task, OC_STACK_KEEP_TRANSACTION, NSOICFree(addr)); NSConsumerPushEvent(task); } @@ -76,6 +76,7 @@ OCStackApplicationResult NSConsumerPresenceListener( else if (payload->trigger == OC_PRESENCE_TRIGGER_CREATE) { NS_LOG(DEBUG, "started presence or resource is created."); + NS_VERIFY_NOT_NULL(clientResponse->addr, OC_STACK_KEEP_TRANSACTION); NSInvokeRequest(NULL, OC_REST_DISCOVER, clientResponse->addr, NS_DISCOVER_QUERY, NULL, NSProviderDiscoverListener, NULL, NULL, clientResponse->addr->adapter); diff --git a/service/notification/src/provider/NSProviderListener.c b/service/notification/src/provider/NSProviderListener.c index 5d9adce..b6e7872 100644 --- a/service/notification/src/provider/NSProviderListener.c +++ b/service/notification/src/provider/NSProviderListener.c @@ -110,6 +110,7 @@ OCEntityHandlerResult NSEntityHandlerMessageCb(OCEntityHandlerFlag flag, && strcmp(reqInterface, NS_INTERFACE_READ) != 0) { NS_LOG(ERROR, "Invalid interface"); + NSOICFree(reqInterface); return ehResult; } ehResult = OC_EH_OK; @@ -185,6 +186,7 @@ OCEntityHandlerResult NSEntityHandlerSyncCb(OCEntityHandlerFlag flag, && strcmp(reqInterface, NS_INTERFACE_READWRITE) != 0) { NS_LOG(ERROR, "Invalid interface"); + NSOICFree(reqInterface); return ehResult; } @@ -279,6 +281,7 @@ OCEntityHandlerResult NSEntityHandlerTopicCb(OCEntityHandlerFlag flag, && strcmp(reqInterface, NS_INTERFACE_READWRITE) != 0) { NS_LOG(ERROR, "Invalid interface"); + NSOICFree(reqInterface); return ehResult; } // send consumer's interesting topic list if consumer id exists diff --git a/service/notification/src/provider/NSProviderSubscription.c b/service/notification/src/provider/NSProviderSubscription.c index 220dced..d5d54e9 100644 --- a/service/notification/src/provider/NSProviderSubscription.c +++ b/service/notification/src/provider/NSProviderSubscription.c @@ -257,6 +257,7 @@ NSResult NSSendResponse(const char * id, bool accepted) if (NSPutMessageResource(NULL, &rHandle) != NS_OK) { NS_LOG(ERROR, "Fail to put notification resource"); + OCRepPayloadDestroy(payload); return NS_ERROR; } @@ -270,6 +271,7 @@ NSResult NSSendResponse(const char * id, bool accepted) if (element == NULL) { NS_LOG(ERROR, "element is NULL"); + OCRepPayloadDestroy(payload); return NS_ERROR; } diff --git a/service/notification/src/provider/NSProviderTopic.c b/service/notification/src/provider/NSProviderTopic.c index 4298939..9e94558 100644 --- a/service/notification/src/provider/NSProviderTopic.c +++ b/service/notification/src/provider/NSProviderTopic.c @@ -133,6 +133,7 @@ NSResult NSSendTopicUpdation() if (NSPutMessageResource(NULL, &rHandle) != NS_OK) { NS_LOG(ERROR, "Fail to put message resource"); + OCRepPayloadDestroy(payload); return NS_ERROR; } @@ -164,6 +165,7 @@ NSResult NSSendTopicUpdation() if (!obCount) { NS_LOG(ERROR, "observer count is zero"); + OCRepPayloadDestroy(payload); return NS_ERROR; } @@ -196,6 +198,7 @@ NSResult NSSendTopicUpdationToConsumer(char *consumerId) if (NSPutMessageResource(NULL, &rHandle) != NS_OK) { NS_LOG(ERROR, "Fail to put message resource"); + OCRepPayloadDestroy(payload); return NS_ERROR; } @@ -208,6 +211,7 @@ NSResult NSSendTopicUpdationToConsumer(char *consumerId) if (element == NULL) { NS_LOG(ERROR, "element is NULL"); + OCRepPayloadDestroy(payload); return NS_ERROR; } @@ -526,6 +530,7 @@ void * NSTopicSchedule(void * ptr) pthread_mutex_lock(topicSyncResult->mutex); topicSyncResult->result = NSUnregisterTopic( (const char *) topicSyncResult->topicData); + NSOICFree(topicSyncResult->topicData); pthread_cond_signal(topicSyncResult->condition); pthread_mutex_unlock(topicSyncResult->mutex); } diff --git a/service/notification/unittest/NSConsumerTest2.cpp b/service/notification/unittest/NSConsumerTest2.cpp index a172ef2..665d89f 100644 --- a/service/notification/unittest/NSConsumerTest2.cpp +++ b/service/notification/unittest/NSConsumerTest2.cpp @@ -169,15 +169,13 @@ namespace OCRepPayload * payload = OCRepPayloadCreate(); EXPECT_NE((void *)NULL, payload); - std::string msgUri = "/notifiationTest/message"; - std::string syncUri = "/notifiationTest/sync"; - std::string topicUri = "/notifiationTest/topic"; + std::string msgUri = "/notifiation/message"; + std::string syncUri = "/notifiation/sync"; bool getResult = OCRepPayloadSetPropBool(payload, NS_ATTRIBUTE_POLICY, false); getResult &= OCRepPayloadSetPropString(payload, NS_ATTRIBUTE_PROVIDER_ID, testProviderID.c_str()); getResult &= OCRepPayloadSetPropString(payload, NS_ATTRIBUTE_MESSAGE, msgUri.c_str()); getResult &= OCRepPayloadSetPropString(payload, NS_ATTRIBUTE_SYNC, syncUri.c_str()); - getResult &= OCRepPayloadSetPropString(payload, NS_ATTRIBUTE_TOPIC, topicUri.c_str()); if (getResult == false) { OCRepPayloadDestroy(payload); @@ -503,7 +501,7 @@ TEST(NotificationConsumerTest, ExpectSuccessSendSyncInfo) uint64_t id = 100; type = NS_SYNC_READ; - auto ret = NSConsumerSendSyncInfo(g_provider->providerId, id, NS_SYNC_DELETED); + auto ret = NSConsumerSendSyncInfo((g_provider->providerId)+1, id, NS_SYNC_DELETED); EXPECT_EQ(NS_OK, ret); } @@ -615,6 +613,7 @@ TEST(NotificationConsumerTest, ExpectUnsubscribeWithPresenceStart) OCPresencePayload * payload = OCPresencePayloadCreate(1, 2, OC_PRESENCE_TRIGGER_CREATE, NULL); EXPECT_NE((void *)NULL, payload); g_testResponse->payload = (OCPayload *)payload; + g_testResponse->addr = NULL; auto ret = NSConsumerPresenceListener(NULL,NULL, g_testResponse); @@ -632,6 +631,9 @@ TEST(NotificationConsumerTest, ExpectUnsubscribeWithPresenceStop) auto ret = NSConsumerPresenceListener(NULL,NULL, g_testResponse); + std::unique_lock< std::mutex > lock{ providerChangedLock }; + providerChanged.wait_for(lock, g_waitForResponse); + EXPECT_EQ(OC_STACK_KEEP_TRANSACTION, ret); OCPresencePayloadDestroy(payload); diff --git a/service/notification/unittest/NSProviderTest2.cpp b/service/notification/unittest/NSProviderTest2.cpp index 98203ad..fb13b87 100644 --- a/service/notification/unittest/NSProviderTest2.cpp +++ b/service/notification/unittest/NSProviderTest2.cpp @@ -130,8 +130,12 @@ namespace bool ret = OCRepPayloadSetPropInt(payload, NS_ATTRIBUTE_MESSAGE_ID, id); OCUUIdentity provider; OC::OCPlatform::getDeviceId(&provider); + + char providerId[UUID_IDENTITY_SIZE] = {0,}; + OICStrcpy(providerId, UUID_IDENTITY_SIZE, (const char *)provider.id); + ret &= OCRepPayloadSetPropString(payload, - NS_ATTRIBUTE_PROVIDER_ID, (const char *)provider.id); + NS_ATTRIBUTE_PROVIDER_ID, (const char*)providerId); ret &= OCRepPayloadSetPropInt(payload, NS_ATTRIBUTE_STATE, NS_SYNC_READ); EXPECT_EQ(true, ret); @@ -307,13 +311,17 @@ TEST(NotificationProviderTest, ExpectCallbackReceiveSync) int type = NS_SYNC_READ; OCEntityHandlerFlag flag = OC_REQUEST_FLAG; - NSEntityHandlerSyncCb(flag, getPostSyncEntityRequest(id), NULL); + auto request = getPostSyncEntityRequest(id); + NSEntityHandlerSyncCb(flag, request, NULL); std::unique_lock< std::mutex > lock{ responseProviderSyncLock }; responseProviderSync.wait_for(lock, g_waitForResponse); EXPECT_EQ(expectedMsgId, id); EXPECT_EQ(expectedSyncType, type); + + OCRepPayloadDestroy((OCRepPayload *)request->payload); + free(request); } TEST(NotificationProviderTest, ExpectSuccessSetTopics) -- 2.7.4