This is an automated email from the ASF dual-hosted git repository. pnoltes pushed a commit to branch hotfix/dm_cmp_coverity_issues in repository https://gitbox.apache.org/repos/asf/celix.git
commit 0612591ca658ed5a9fdfc295255c366f8b2b45cd Author: Pepijn Noltes <[email protected]> AuthorDate: Tue Jun 7 17:05:35 2022 +0200 Fixes several mem leak issues, detected by coverity --- .../gtest/src/CxxBundleContextTestSuite.cc | 12 ++++-- .../gtest/src/DependencyManagerTestSuite.cc | 24 ++++++++++- .../gtest/src/bundle_context_bundles_tests.cpp | 20 ++++----- libs/framework/include/service_registry.h | 20 ++++----- libs/framework/src/dm_component_impl.c | 38 +++++++--------- libs/framework/src/dm_dependency_manager_impl.c | 16 +++++++ libs/framework/src/framework.c | 4 +- libs/framework/src/service_registry.c | 50 ++++++++-------------- 8 files changed, 102 insertions(+), 82 deletions(-) diff --git a/libs/framework/gtest/src/CxxBundleContextTestSuite.cc b/libs/framework/gtest/src/CxxBundleContextTestSuite.cc index ec97259b..c5d409d3 100644 --- a/libs/framework/gtest/src/CxxBundleContextTestSuite.cc +++ b/libs/framework/gtest/src/CxxBundleContextTestSuite.cc @@ -766,12 +766,16 @@ TEST_F(CxxBundleContextTestSuite, TestOldCStyleTrackerWithCxxMetaTracker) { service_tracker_customizer_t *customizer = nullptr; auto status = serviceTrackerCustomizer_create(this, nullptr, nullptr, nullptr, nullptr, &customizer); - ASSERT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(status, CELIX_SUCCESS); + celix_service_tracker_t* tracker = nullptr; status = serviceTracker_createWithFilter(ctx->getCBundleContext(), "(service.exported.interfaces=*)", customizer, &tracker); - ASSERT_EQ(status, CELIX_SUCCESS); - status = serviceTracker_open(tracker); - ASSERT_EQ(status, CELIX_SUCCESS); + EXPECT_EQ(status, CELIX_SUCCESS); + + if (status == CELIX_SUCCESS) { + status = serviceTracker_open(tracker); + EXPECT_EQ(status, CELIX_SUCCESS); + } auto metaTracker = ctx->trackAnyServiceTrackers().build(); ctx->waitForEvents(); diff --git a/libs/framework/gtest/src/DependencyManagerTestSuite.cc b/libs/framework/gtest/src/DependencyManagerTestSuite.cc index 29ce998a..d9939fbb 100644 --- a/libs/framework/gtest/src/DependencyManagerTestSuite.cc +++ b/libs/framework/gtest/src/DependencyManagerTestSuite.cc @@ -115,13 +115,33 @@ TEST_F(DependencyManagerTestSuite, DmComponentRemoveAllAsync) { EXPECT_EQ(2, celix_dependencyManager_nrOfComponents(mng)); std::atomic<std::size_t> count{0}; - celix_dependencyManager_removeAllComponentsAsync(mng, &count, [](void *data) { + auto callback = [](void *data) { auto* c = static_cast<std::atomic<std::size_t>*>(data); c->fetch_add(1); - }); + }; + celix_dependencyManager_removeAllComponentsAsync(mng, &count, callback); celix_dependencyManager_wait(mng); EXPECT_EQ(0, celix_dependencyManager_nrOfComponents(mng)); EXPECT_EQ(1, count.load()); + + //call again if all components are removed -> should call callback + celix_dependencyManager_removeAllComponentsAsync(mng, &count, callback); + celix_dependencyManager_wait(mng); + EXPECT_EQ(0, celix_dependencyManager_nrOfComponents(mng)); + EXPECT_EQ(2, count.load()); +} + +TEST_F(DependencyManagerTestSuite, InvalidAddInterface) { + auto* cmp = celix_dmComponent_create(ctx, "test"); + void* dummyInterfacePointer = (void*)0x42; + + auto status = celix_dmComponent_addInterface(cmp, "", nullptr, dummyInterfacePointer, nullptr); + EXPECT_NE(status, CELIX_SUCCESS); + + status = celix_dmComponent_addInterface(cmp, nullptr, nullptr, dummyInterfacePointer, nullptr); + EXPECT_NE(status, CELIX_SUCCESS); + + celix_dmComponent_destroy(cmp); } TEST_F(DependencyManagerTestSuite, DmGetInfo) { diff --git a/libs/framework/gtest/src/bundle_context_bundles_tests.cpp b/libs/framework/gtest/src/bundle_context_bundles_tests.cpp index 0d3fc4d0..f63c4348 100644 --- a/libs/framework/gtest/src/bundle_context_bundles_tests.cpp +++ b/libs/framework/gtest/src/bundle_context_bundles_tests.cpp @@ -220,45 +220,45 @@ TEST_F(CelixBundleContextBundlesTests, StopStartTest) { celix_array_list_t *ids = celix_bundleContext_listInstalledBundles(ctx); size_t size = celix_arrayList_size(ids); - ASSERT_EQ(3, size); + EXPECT_EQ(3, size); celix_arrayList_destroy(ids); ids = celix_bundleContext_listBundles(ctx); size = celix_arrayList_size(ids); - ASSERT_EQ(3, size); + EXPECT_EQ(3, size); int count = 0; celix_bundleContext_useBundles(ctx, &count, [](void *handle, const celix_bundle_t *bnd) { auto *c = (int*)handle; - ASSERT_EQ(OSGI_FRAMEWORK_BUNDLE_ACTIVE, celix_bundle_getState(bnd)); + EXPECT_EQ(OSGI_FRAMEWORK_BUNDLE_ACTIVE, celix_bundle_getState(bnd)); *c += 1; }); - ASSERT_EQ(3, count); + EXPECT_EQ(3, count); for (size_t i = 0; i < size; ++i) { bool stopped = celix_bundleContext_stopBundle(ctx, celix_arrayList_getLong(ids, (int)i)); - ASSERT_TRUE(stopped); + EXPECT_TRUE(stopped); } bool stopped = celix_bundleContext_stopBundle(ctx, 42 /*non existing*/); - ASSERT_FALSE(stopped); + EXPECT_FALSE(stopped); bool started = celix_bundleContext_startBundle(ctx, 42 /*non existing*/); - ASSERT_FALSE(started); + EXPECT_FALSE(started); for (size_t i = 0; i < size; ++i) { started = celix_bundleContext_startBundle(ctx, celix_arrayList_getLong(ids, (int)i)); - ASSERT_TRUE(started); + EXPECT_TRUE(started); } count = 0; celix_bundleContext_useBundles(ctx, &count, [](void *handle, const celix_bundle_t *bnd) { auto *c = (int*)handle; - ASSERT_EQ(OSGI_FRAMEWORK_BUNDLE_ACTIVE, celix_bundle_getState(bnd)); + EXPECT_EQ(OSGI_FRAMEWORK_BUNDLE_ACTIVE, celix_bundle_getState(bnd)); *c += 1; }); - ASSERT_EQ(3, count); + EXPECT_EQ(3, count); celix_arrayList_destroy(ids); } diff --git a/libs/framework/include/service_registry.h b/libs/framework/include/service_registry.h index 9c0ede63..d2fe3227 100644 --- a/libs/framework/include/service_registry.h +++ b/libs/framework/include/service_registry.h @@ -38,36 +38,36 @@ extern "C" { typedef void (*serviceChanged_function_pt)(celix_framework_t*, celix_service_event_type_t, service_registration_pt, celix_properties_t*); -celix_status_t serviceRegistry_create(celix_framework_t *framework, service_registry_pt *registry); +celix_service_registry_t* celix_serviceRegistry_create(celix_framework_t *framework); -celix_status_t serviceRegistry_destroy(service_registry_pt registry); +void celix_serviceRegistry_destroy(celix_service_registry_t* registry); celix_status_t -serviceRegistry_getRegisteredServices(service_registry_pt registry, celix_bundle_t *bundle, celix_array_list_t **services); +serviceRegistry_getRegisteredServices(celix_service_registry_t* registry, celix_bundle_t *bundle, celix_array_list_t **services); celix_status_t -serviceRegistry_getServicesInUse(service_registry_pt registry, celix_bundle_t *bundle, celix_array_list_t **services); +serviceRegistry_getServicesInUse(celix_service_registry_t* registry, celix_bundle_t *bundle, celix_array_list_t **services); -celix_status_t serviceRegistry_registerService(service_registry_pt registry, celix_bundle_t *bundle, const char *serviceName, +celix_status_t serviceRegistry_registerService(celix_service_registry_t* registry, celix_bundle_t *bundle, const char *serviceName, const void *serviceObject, celix_properties_t *dictionary, service_registration_pt *registration); celix_status_t -serviceRegistry_registerServiceFactory(service_registry_pt registry, celix_bundle_t *bundle, const char *serviceName, +serviceRegistry_registerServiceFactory(celix_service_registry_t* registry, celix_bundle_t *bundle, const char *serviceName, service_factory_pt factory, celix_properties_t *dictionary, service_registration_pt *registration); -celix_status_t serviceRegistry_getServiceReference(service_registry_pt registry, celix_bundle_t *bundle, +celix_status_t serviceRegistry_getServiceReference(celix_service_registry_t* registry, celix_bundle_t *bundle, service_registration_pt registration, service_reference_pt *reference); celix_status_t -serviceRegistry_getServiceReferences(service_registry_pt registry, celix_bundle_t *bundle, const char *serviceName, +serviceRegistry_getServiceReferences(celix_service_registry_t* registry, celix_bundle_t *bundle, const char *serviceName, filter_pt filter, celix_array_list_t **references); -celix_status_t serviceRegistry_clearReferencesFor(service_registry_pt registry, celix_bundle_t *bundle); +celix_status_t serviceRegistry_clearReferencesFor(celix_service_registry_t* registry, celix_bundle_t *bundle); -size_t serviceRegistry_nrOfHooks(service_registry_pt registry); +size_t serviceRegistry_nrOfHooks(celix_service_registry_t* registry); /** * Register a service listener. Will also retroactively call the listener with register events for already registered services. diff --git a/libs/framework/src/dm_component_impl.c b/libs/framework/src/dm_component_impl.c index a951efc1..d8838bba 100644 --- a/libs/framework/src/dm_component_impl.c +++ b/libs/framework/src/dm_component_impl.c @@ -422,10 +422,13 @@ celix_status_t component_addInterface(celix_dm_component_t *component, const cha } celix_status_t celix_dmComponent_addInterface(celix_dm_component_t *component, const char* serviceName, const char* serviceVersion, const void* service, celix_properties_t* properties) { - celix_status_t status = CELIX_SUCCESS; + if (serviceName == NULL || celix_utils_stringEquals(serviceName, "")) { + celix_bundleContext_log(component->context, CELIX_LOG_LEVEL_ERROR, "Cannot add interface with a NULL or empty serviceName"); + return CELIX_ILLEGAL_ARGUMENT; + } - dm_interface_t *interface = (dm_interface_t *) calloc(1, sizeof(*interface)); - char *name = strdup(serviceName); + dm_interface_t *interface = calloc(1, sizeof(*interface)); + char *name = celix_utils_strdup(serviceName); if (properties == NULL) { properties = celix_properties_create(); @@ -434,27 +437,20 @@ celix_status_t celix_dmComponent_addInterface(celix_dm_component_t *component, c if ((properties_get(properties, CELIX_FRAMEWORK_SERVICE_VERSION) == NULL) && (serviceVersion != NULL)) { celix_properties_set(properties, CELIX_FRAMEWORK_SERVICE_VERSION, serviceVersion); } - celix_properties_set(properties, CELIX_DM_COMPONENT_UUID, (char*)component->uuid); - if (interface && name) { - celixThreadMutex_lock(&component->mutex); - interface->serviceName = name; - interface->service = service; - interface->properties = properties; - interface->svcId= -1L; - celix_arrayList_add(component->providedInterfaces, interface); - if (celix_dmComponent_currentState(component) == CELIX_DM_CMP_STATE_TRACKING_OPTIONAL) { - celix_dmComponent_registerServices(component, false); - } - celixThreadMutex_unlock(&component->mutex); - } else { - free(interface); - free(name); - status = CELIX_ENOMEM; + celixThreadMutex_lock(&component->mutex); + interface->serviceName = name; + interface->service = service; + interface->properties = properties; + interface->svcId= -1L; + celix_arrayList_add(component->providedInterfaces, interface); + if (celix_dmComponent_currentState(component) == CELIX_DM_CMP_STATE_TRACKING_OPTIONAL) { + celix_dmComponent_registerServices(component, false); } + celixThreadMutex_unlock(&component->mutex); - return status; + return CELIX_SUCCESS; } celix_status_t component_removeInterface(celix_dm_component_t *component, const void* service) { @@ -572,9 +568,7 @@ static celix_status_t celix_dmComponent_resume(celix_dm_component_t *component, "Error starting component %s (uuid=%s) using the start callback. Disabling component.", component->name, component->uuid); - celixThreadMutex_lock(&component->mutex); celix_dmComponent_disableDirectly(component); - celixThreadMutex_unlock(&component->mutex); } celixThreadMutex_unlock(&component->mutex); } diff --git a/libs/framework/src/dm_dependency_manager_impl.c b/libs/framework/src/dm_dependency_manager_impl.c index 594d93c2..1277a2f2 100644 --- a/libs/framework/src/dm_dependency_manager_impl.c +++ b/libs/framework/src/dm_dependency_manager_impl.c @@ -148,6 +148,22 @@ celix_status_t celix_dependencyManager_removeAllComponentsAsync(celix_dependency if (doneCallback != NULL) { callbackData->destroysInProgress = celix_arrayList_size(manager->components); } + if (celix_arrayList_size(manager->components) == 0 && doneCallback != NULL) { + //corner case: if no components are available, call done callback on event thread. + celix_framework_t* fw = celix_bundleContext_getFramework(manager->ctx); + long bndId = celix_bundleContext_getBundleId(manager->ctx); + celix_framework_fireGenericEvent( + fw, + -1, + bndId, + "celix_dependencyManager_removeAllComponentsAsync callback", + doneData, + doneCallback, + NULL, + NULL + ); + free(callbackData); + } for (int i = 0; i < celix_arrayList_size(manager->components); ++i) { celix_dm_component_t *cmp = celix_arrayList_get(manager->components, i); if (doneCallback != NULL) { diff --git a/libs/framework/src/framework.c b/libs/framework/src/framework.c index c2ec40bb..6e5f77d4 100644 --- a/libs/framework/src/framework.c +++ b/libs/framework/src/framework.c @@ -263,7 +263,7 @@ celix_status_t framework_create(framework_pt *out, celix_properties_t* config) { status = CELIX_DO_IF(status, bundle_getBundleId(framework->bundle, &framework->bundleId)); status = CELIX_DO_IF(status, bundle_setFramework(framework->bundle, framework)); status = CELIX_DO_IF(status, bundleCache_create(uuid, framework->configurationMap, &framework->cache)); - status = CELIX_DO_IF(status, serviceRegistry_create(framework, &framework->registry)); + framework->registry = celix_serviceRegistry_create(framework); bundle_context_t *context = NULL; status = CELIX_DO_IF(status, bundleContext_create(framework, framework->logger, framework->bundle, &context)); status = CELIX_DO_IF(status, bundle_setContext(framework->bundle, context)); @@ -314,7 +314,7 @@ celix_status_t framework_destroy(framework_pt framework) { } - serviceRegistry_destroy(framework->registry); + celix_serviceRegistry_destroy(framework->registry); celixThreadMutex_lock(&framework->installedBundles.mutex); for (int i = 0; i < celix_arrayList_size(framework->installedBundles.entries); ++i) { diff --git a/libs/framework/src/service_registry.c b/libs/framework/src/service_registry.c index cf03ee13..4de6ee02 100644 --- a/libs/framework/src/service_registry.c +++ b/libs/framework/src/service_registry.c @@ -54,44 +54,32 @@ static void celix_increasePendingRegisteredEvent(celix_service_registry_t *regis static void celix_decreasePendingRegisteredEvent(celix_service_registry_t *registry, long svcId); static void celix_waitForPendingRegisteredEvents(celix_service_registry_t *registry, long svcId); -celix_status_t serviceRegistry_create(framework_pt framework, service_registry_pt *out) { - celix_status_t status; - - service_registry_pt reg = calloc(1, sizeof(*reg)); - if (reg == NULL) { - status = CELIX_ENOMEM; - } else { +celix_service_registry_t* celix_serviceRegistry_create(framework_pt framework) { + celix_service_registry_t* reg = calloc(1, sizeof(*reg)); - reg->callback.handle = reg; - reg->callback.getUsingBundles = (void *)serviceRegistry_getUsingBundles; - reg->callback.unregister = (void *) serviceRegistry_unregisterService; - reg->callback.tryRemoveServiceReference = (void *) serviceRegistry_tryRemoveServiceReference; + reg->callback.handle = reg; + reg->callback.getUsingBundles = (void *)serviceRegistry_getUsingBundles; + reg->callback.unregister = (void *) serviceRegistry_unregisterService; + reg->callback.tryRemoveServiceReference = (void *) serviceRegistry_tryRemoveServiceReference; - reg->serviceRegistrations = hashMap_create(NULL, NULL, NULL, NULL); - reg->framework = framework; - reg->nextServiceId = 1L; - reg->serviceReferences = hashMap_create(NULL, NULL, NULL, NULL); + reg->serviceRegistrations = hashMap_create(NULL, NULL, NULL, NULL); + reg->framework = framework; + reg->nextServiceId = 1L; + reg->serviceReferences = hashMap_create(NULL, NULL, NULL, NULL); - reg->listenerHooks = celix_arrayList_create(); - reg->serviceListeners = celix_arrayList_create(); + reg->listenerHooks = celix_arrayList_create(); + reg->serviceListeners = celix_arrayList_create(); - celixThreadMutex_create(®->pendingRegisterEvents.mutex, NULL); - celixThreadCondition_init(®->pendingRegisterEvents.cond, NULL); - reg->pendingRegisterEvents.map = hashMap_create(NULL, NULL, NULL, NULL); - - status = celixThreadRwlock_create(®->lock, NULL); - } + celixThreadMutex_create(®->pendingRegisterEvents.mutex, NULL); + celixThreadCondition_init(®->pendingRegisterEvents.cond, NULL); + celixThreadRwlock_create(®->lock, NULL); + reg->pendingRegisterEvents.map = hashMap_create(NULL, NULL, NULL, NULL); - if (status == CELIX_SUCCESS) { - *out = reg; - } else { - framework_logIfError(framework->logger, status, NULL, "Cannot create service registry"); - } - return status; + return reg; } -celix_status_t serviceRegistry_destroy(service_registry_pt registry) { +void celix_serviceRegistry_destroy(celix_service_registry_t* registry) { celixThreadRwlock_writeLock(®istry->lock); //remove service listeners @@ -149,8 +137,6 @@ celix_status_t serviceRegistry_destroy(service_registry_pt registry) { hashMap_destroy(registry->pendingRegisterEvents.map, false, false); free(registry); - - return CELIX_SUCCESS; } celix_status_t serviceRegistry_getRegisteredServices(service_registry_pt registry, bundle_pt bundle, array_list_pt *services) {
