From 47aca923ac0db2cd2e888cf45bedd326459d7e89 Mon Sep 17 00:00:00 2001 From: Poovizhi Date: Fri, 3 Mar 2017 15:54:09 +0530 Subject: [PATCH] [IOT-1880] Adding Fixes for issues generated from static analyzer tool in Notification service c++ , JNI layer 1) In JNINotificationConsumer.cpp, Getting native Provider object is common code for all the native methods. Added a separate method ' getNativeProvider' to do this, so that method size and complexity will be reduced. 2) JNiNotificationConsumer.cpp Line 1379, and NSTopicslist.cpp Line 59 has changes to fix the issue of Unreachable code. 3) Copy constructor and copy assignment operator are added in class 'NSAcceptedProviders' & 'NSAcceptedConsumers' which has dynamically allocated data members 4) In NSAcceptedProviders class, getProviders() method is changed to const since it is being used by the copy constructors and hence modified the member variable 'm_mutex' to be mutable. 5) In NSAcceptedConsumers class, getConsumers() method is changed to const since it is being used by the copy constructors and hence modified the member variable 'm_mutex' to be mutable. 6) In NotiListener.java, the NULL check for mProviderSample is moved above the first instance where mProviderSample is beig used. Change-Id: Ic18c3d9797a02a73f5397192b21e7dda5926119e Signed-off-by: Poovizhi Reviewed-on: https://gerrit.iotivity.org/gerrit/17653 Reviewed-by: Uze Choi Tested-by: Uze Choi --- .../main/jni/consumer/JniNotificationConsumer.cpp | 146 +++++---------------- .../cpp-wrapper/common/NSTopicsList.cpp | 10 +- .../cpp-wrapper/consumer/inc/NSAcceptedProviders.h | 17 ++- .../consumer/src/NSAcceptedProviders.cpp | 15 ++- .../cpp-wrapper/consumer/src/NSConsumerService.cpp | 5 +- .../cpp-wrapper/provider/inc/NSAcceptedConsumers.h | 17 ++- .../provider/src/NSAcceptedConsumers.cpp | 15 ++- .../service/ns/sample/provider/NotiListener.java | 10 +- 8 files changed, 106 insertions(+), 129 deletions(-) mode change 100755 => 100644 service/notification/android/notification-service/src/main/jni/consumer/JniNotificationConsumer.cpp mode change 100755 => 100644 service/notification/cpp-wrapper/common/NSTopicsList.cpp mode change 100755 => 100644 service/notification/cpp-wrapper/consumer/inc/NSAcceptedProviders.h mode change 100755 => 100644 service/notification/cpp-wrapper/consumer/src/NSAcceptedProviders.cpp mode change 100755 => 100644 service/notification/cpp-wrapper/consumer/src/NSConsumerService.cpp mode change 100755 => 100644 service/notification/cpp-wrapper/provider/inc/NSAcceptedConsumers.h mode change 100755 => 100644 service/notification/cpp-wrapper/provider/src/NSAcceptedConsumers.cpp diff --git a/service/notification/android/notification-service/src/main/jni/consumer/JniNotificationConsumer.cpp b/service/notification/android/notification-service/src/main/jni/consumer/JniNotificationConsumer.cpp old mode 100755 new mode 100644 index 7abe708..a9eb888 --- a/service/notification/android/notification-service/src/main/jni/consumer/JniNotificationConsumer.cpp +++ b/service/notification/android/notification-service/src/main/jni/consumer/JniNotificationConsumer.cpp @@ -71,6 +71,24 @@ static JNIEnv *GetJNIEnv(jint *ret) } } +static jlong getNativeProvider(JNIEnv *env, jobject jObj) +{ + + jclass providerClass = env->GetObjectClass(jObj); + if (!providerClass) + { + ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); + return 0; + } + jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); + if (!nativeHandle) + { + ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); + return 0; + } + return (env->GetLongField(jObj, nativeHandle)); +} + jobject getJavaProviderState(JNIEnv *env, OIC::Service::NSProviderState state) { NS_LOGD ("ConsumerService_getJavaProviderState - IN"); @@ -297,7 +315,7 @@ const char *getNativeTopicName(JNIEnv *env, jobject jTopic) } else { - NS_LOGI (TAG, "Info: topicName is null"); + NS_LOGI ("topicName is null"); } NS_LOGD ("ConsumerService_getNativeTopicName - OUT"); return topicName; @@ -394,6 +412,7 @@ jobject getJavaProvider(JNIEnv *env, std::shared_ptr p if (!cls_provider) { NS_LOGE ("Failed to Get ObjectClass for Provider"); + delete objectHolder; return NULL; } jmethodID mid_provider = env->GetMethodID( @@ -401,12 +420,14 @@ jobject getJavaProvider(JNIEnv *env, std::shared_ptr p if (!mid_provider) { NS_LOGE ("Failed to Get MethodID for Provider"); + delete objectHolder; return NULL; } jobject obj_provider = env->NewObject(cls_provider, mid_provider, jProviderId); if (!obj_provider) { NS_LOGE ("Failed to create new Object for Provider"); + delete objectHolder; return NULL; } @@ -414,6 +435,7 @@ jobject getJavaProvider(JNIEnv *env, std::shared_ptr p if (!nativeHandle) { NS_LOGE ("Failed to get nativeHandle for Provider"); + delete objectHolder; return NULL; } env->SetLongField(obj_provider, nativeHandle, pProvider); @@ -1066,20 +1088,8 @@ JNIEXPORT void JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeSubs { NS_LOGD ("Provider_Subscribe -IN"); OIC::Service::NSResult result = OIC::Service::NSResult::ERROR; - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return ; - } - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return ; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); if (jProvider) { NS_LOGD ("calling subscribe on mNativeHandle"); @@ -1114,20 +1124,7 @@ JNIEXPORT void JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeUnsu { NS_LOGD ("Provider_UnSubscribe -IN"); OIC::Service::NSResult result = OIC::Service::NSResult::ERROR; - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return ; - } - - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return ; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); if (jProvider) { NS_LOGD ("calling subscribe on mNativeHandle"); @@ -1168,25 +1165,12 @@ JNIEXPORT void JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeSend return ; } - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return ; - } - - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return ; - } uint64_t messageId = (uint64_t) jMessageId; NS_LOGD ("!!!!!!jMessageId: %lld", jMessageId); NS_LOGD ("!!!!!!messageId: %lld", messageId); - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); if (jProvider) { NS_LOGD ("calling SendSyncInfo on mNativeHandle"); @@ -1228,20 +1212,7 @@ JNIEXPORT void JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeSetL return ; } - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return ; - } - - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return ; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); if (jProvider) { NS_LOGD ("calling SetListener on mNativeHandle"); @@ -1278,20 +1249,7 @@ JNIEXPORT jobject JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeG (JNIEnv *env, jobject jObj) { NS_LOGD ("Provider_nativeGetTopicList - IN"); - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return NULL; - } - - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return NULL; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); std::shared_ptr topicList = nullptr; if (jProvider) { @@ -1341,20 +1299,7 @@ JNIEXPORT void JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeUpda return; } - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return; - } - - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); OIC::Service::NSResult result = OIC::Service::NSResult::ERROR; if (jProvider) { @@ -1389,20 +1334,7 @@ JNIEXPORT jobject JNICALL Java_org_iotivity_service_ns_consumer_Provider_nativeG (JNIEnv *env, jobject jObj) { NS_LOGD ("Provider_nativeGetProviderState - IN"); - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return NULL; - } - - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return NULL; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); OIC::Service::NSProviderState state = OIC::Service::NSProviderState::DENY; if (jProvider) { @@ -1434,20 +1366,9 @@ JNIEXPORT jboolean JNICALL Java_org_iotivity_service_ns_consumer_Provider_native (JNIEnv *env, jobject jObj) { NS_LOGD ("nativeIsSubscribed - IN"); - jclass providerClass = env->GetObjectClass(jObj); - if (!providerClass) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to Get ObjectClass for Provider"); - return (jboolean)false; - } + jboolean subscribedState = false; - jfieldID nativeHandle = env->GetFieldID(providerClass, "mNativeHandle", "J"); - if (!nativeHandle) - { - ThrowNSException(JNI_INVALID_VALUE, "Failed to get nativeHandle for Provider"); - return (jboolean)false; - } - jlong jProvider = env->GetLongField(jObj, nativeHandle); + jlong jProvider = getNativeProvider(env, jObj); if (jProvider) { NS_LOGD ("calling isSubscribe on mNativeHandle"); @@ -1455,13 +1376,14 @@ JNIEXPORT jboolean JNICALL Java_org_iotivity_service_ns_consumer_Provider_native reinterpret_cast *>(jProvider); try { - return (jboolean) objectHolder->get()->isSubscribed(); + subscribedState = (jboolean)objectHolder->get()->isSubscribed(); } catch (OIC::Service::NSException ex) { ThrowNSException(NATIVE_EXCEPTION, ex.what()); return (jboolean)false; } + return subscribedState; } else { diff --git a/service/notification/cpp-wrapper/common/NSTopicsList.cpp b/service/notification/cpp-wrapper/common/NSTopicsList.cpp old mode 100755 new mode 100644 index c9a6970..6235833 --- a/service/notification/cpp-wrapper/common/NSTopicsList.cpp +++ b/service/notification/cpp-wrapper/common/NSTopicsList.cpp @@ -35,7 +35,7 @@ namespace OIC while (topicsNode != nullptr) { m_topicsList.push_back(new NSTopic( - topicsNode->topicName, (NSTopic::NSTopicState)topicsNode->state)); + topicsNode->topicName, (NSTopic::NSTopicState)topicsNode->state)); topicsNode = topicsNode->next; } @@ -55,8 +55,8 @@ namespace OIC { this->m_topicsList.push_back(new NSTopic(it.getTopicName(), it.getState())); } - return *this; m_modifiable = false; + return *this; } NSTopicsList::~NSTopicsList() @@ -70,7 +70,7 @@ namespace OIC void NSTopicsList::addTopic(const std::string &topicName, NSTopic::NSTopicState state) { - if(m_modifiable) + if (m_modifiable) { m_topicsList.push_back(new NSTopic(topicName, state)); } @@ -82,7 +82,7 @@ namespace OIC void NSTopicsList::removeTopic(const std::string &topicName) { - if(m_modifiable) + if (m_modifiable) { for (auto it : m_topicsList) { @@ -108,7 +108,7 @@ namespace OIC } return topicList; } - + //Below method restricts the application from illegally modifying Topics when //Provider is in Invalid state. By calling the API, the service prevents and protects //the integrity of TopicsList updation when the associated object is Invalid diff --git a/service/notification/cpp-wrapper/consumer/inc/NSAcceptedProviders.h b/service/notification/cpp-wrapper/consumer/inc/NSAcceptedProviders.h old mode 100755 new mode 100644 index 3922661..0d79de8 --- a/service/notification/cpp-wrapper/consumer/inc/NSAcceptedProviders.h +++ b/service/notification/cpp-wrapper/consumer/inc/NSAcceptedProviders.h @@ -55,6 +55,19 @@ namespace OIC } /** + * Copy Constructor of NSAcceptedProviders. + * + */ + NSAcceptedProviders(const NSAcceptedProviders &); + + /** + * Copy assignment operator of NSAcceptedProviders. + * + * @return NSAcceptedProviders object reference + */ + NSAcceptedProviders &operator=(const NSAcceptedProviders &); + + /** * Destructor of NSAcceptedProviders. */ ~NSAcceptedProviders() @@ -107,11 +120,11 @@ namespace OIC * get the map of providers accepted. * @return m_providers -map of accepted providers */ - std::map > getProviders(); + std::map > getProviders() const; private : std::map > m_providers; - std::mutex m_mutex; + mutable std::mutex m_mutex; }; } } diff --git a/service/notification/cpp-wrapper/consumer/src/NSAcceptedProviders.cpp b/service/notification/cpp-wrapper/consumer/src/NSAcceptedProviders.cpp old mode 100755 new mode 100644 index ac2408c..1e3edf5 --- a/service/notification/cpp-wrapper/consumer/src/NSAcceptedProviders.cpp +++ b/service/notification/cpp-wrapper/consumer/src/NSAcceptedProviders.cpp @@ -27,6 +27,19 @@ namespace OIC { namespace Service { + NSAcceptedProviders::NSAcceptedProviders(const NSAcceptedProviders &providers) + { + removeProviders(); + m_providers.insert((providers.getProviders()).begin(), (providers.getProviders()).end()); + } + + NSAcceptedProviders &NSAcceptedProviders::operator=(const NSAcceptedProviders &providers) + { + removeProviders(); + this->m_providers.insert((providers.getProviders()).begin(), (providers.getProviders()).end()); + return *this; + } + std::shared_ptr NSAcceptedProviders::getProvider(const std::string &id) { std::lock_guard lock(m_mutex); @@ -87,7 +100,7 @@ namespace OIC return; } - std::map > NSAcceptedProviders::getProviders() + std::map > NSAcceptedProviders::getProviders() const { std::lock_guard lock(m_mutex); return m_providers; diff --git a/service/notification/cpp-wrapper/consumer/src/NSConsumerService.cpp b/service/notification/cpp-wrapper/consumer/src/NSConsumerService.cpp old mode 100755 new mode 100644 index d242919..fdfac60 --- a/service/notification/cpp-wrapper/consumer/src/NSConsumerService.cpp +++ b/service/notification/cpp-wrapper/consumer/src/NSConsumerService.cpp @@ -147,7 +147,10 @@ namespace OIC else if (state == NS_STOPPED) { oldProvider->setProviderSubscribedState(NSProviderSubscribedState::DENY); - oldProvider->getTopicList()->unsetModifiability(); + if (NULL != oldProvider->getTopicList()) + { + oldProvider->getTopicList()->unsetModifiability(); + } NSConsumerService::getInstance()->getAcceptedProviders()->removeProvider( oldProvider->getProviderId()); NS_LOG(DEBUG, "initiating the State callback : NS_STOPPED"); diff --git a/service/notification/cpp-wrapper/provider/inc/NSAcceptedConsumers.h b/service/notification/cpp-wrapper/provider/inc/NSAcceptedConsumers.h old mode 100755 new mode 100644 index a57654f..b4ed05d --- a/service/notification/cpp-wrapper/provider/inc/NSAcceptedConsumers.h +++ b/service/notification/cpp-wrapper/provider/inc/NSAcceptedConsumers.h @@ -54,6 +54,19 @@ namespace OIC } /** + * Copy Constructor of NSAcceptedConsumers. + * + */ + NSAcceptedConsumers(const NSAcceptedConsumers &); + + /** + * Copy assignment operator of NSAcceptedConsumers. + * + * @return NSAcceptedConsumers object reference + */ + NSAcceptedConsumers &operator=(const NSAcceptedConsumers &); + + /** * Destructor of NSAcceptedConsumers. */ ~NSAcceptedConsumers() @@ -106,11 +119,11 @@ namespace OIC * get the map of Consumers accepted. * @return m_consumers -map of accepted Consumers */ - std::map > getConsumers(); + std::map > getConsumers() const; private : std::map > m_consumers; - std::mutex m_mutex; + mutable std::mutex m_mutex; }; } } diff --git a/service/notification/cpp-wrapper/provider/src/NSAcceptedConsumers.cpp b/service/notification/cpp-wrapper/provider/src/NSAcceptedConsumers.cpp old mode 100755 new mode 100644 index 241317a..8eda25d --- a/service/notification/cpp-wrapper/provider/src/NSAcceptedConsumers.cpp +++ b/service/notification/cpp-wrapper/provider/src/NSAcceptedConsumers.cpp @@ -29,6 +29,19 @@ namespace OIC { namespace Service { + NSAcceptedConsumers::NSAcceptedConsumers(const NSAcceptedConsumers &consumers) + { + removeConsumers(); + m_consumers.insert((consumers.getConsumers()).begin(), (consumers.getConsumers()).end()); + } + + NSAcceptedConsumers &NSAcceptedConsumers::operator=(const NSAcceptedConsumers &consumers) + { + removeConsumers(); + this->m_consumers.insert((consumers.getConsumers()).begin(), (consumers.getConsumers()).end()); + return *this; + } + std::shared_ptr NSAcceptedConsumers::getConsumer(const std::string &id) { std::lock_guard lock(m_mutex); @@ -89,7 +102,7 @@ namespace OIC return; } - std::map > NSAcceptedConsumers::getConsumers() + std::map > NSAcceptedConsumers::getConsumers() const { std::lock_guard lock(m_mutex); return m_consumers; diff --git a/service/notification/examples/android/NotiProviderExample/app/src/main/java/org/iotivity/service/ns/sample/provider/NotiListener.java b/service/notification/examples/android/NotiProviderExample/app/src/main/java/org/iotivity/service/ns/sample/provider/NotiListener.java index a25e351..02e2395 100755 --- a/service/notification/examples/android/NotiProviderExample/app/src/main/java/org/iotivity/service/ns/sample/provider/NotiListener.java +++ b/service/notification/examples/android/NotiProviderExample/app/src/main/java/org/iotivity/service/ns/sample/provider/NotiListener.java @@ -110,6 +110,10 @@ public class NotiListener extends NotificationListenerService { Log.i(TAG, "Title : " + title); Log.i(TAG, "Body : " + body); + if(mProviderSample == null) { + Log.e(TAG, "mProviderSample NULL"); + return; + } Message notiMessage = mProviderSample.createMessage(); if(notiMessage == null) { @@ -126,11 +130,7 @@ public class NotiListener extends NotificationListenerService { notiMessage.setTime("12:10"); MediaContents media = new MediaContents("daasd"); notiMessage.setMediaContents(media); - if (mProviderSample != null) { - mProviderSample.sendMessage(notiMessage); - } else { - Log.i(TAG, "providerExample is NULL"); - } + mProviderSample.sendMessage(notiMessage); } @Override -- 2.7.4