You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@celix.apache.org by pn...@apache.org on 2022/06/07 15:05:50 UTC
[celix] 01/01: Fixes several mem leak issues, detected by coverity
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 <pn...@apache.org>
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) {