Cleanup bundle cache on installation failure and fix use-after-free.

libs/framework/gtest/CMakeLists.txt

@@ -20,6 +20,8 @@ celix_bundle_name(simple_test_bundle1 "Simple Test Bundle")
 celix_bundle_group(simple_test_bundle1 "test/group")
 celix_bundle_description(simple_test_bundle1 "Test Description")
 
+add_celix_bundle(dup_symbolic_name_bundle NO_ACTIVATOR VERSION 1.0.0 SYMBOLIC_NAME simple_test_bundle1)
+
 add_celix_bundle(simple_test_bundle2 NO_ACTIVATOR VERSION 1.0.0)
 add_celix_bundle(simple_test_bundle3 NO_ACTIVATOR VERSION 1.0.0)
 add_celix_bundle(bundle_with_exception SOURCES src/nop_activator.c VERSION 1.0.0)
@@ -80,11 +82,12 @@ add_celix_bundle_dependencies(test_framework
         simple_test_bundle2 simple_test_bundle3 simple_test_bundle4
         simple_test_bundle5 bundle_with_exception bundle_with_bad_export
         unresolvable_bundle simple_cxx_bundle simple_cxx_dep_man_bundle cmp_test_bundle
-        celix_err_test_bundle)
+        celix_err_test_bundle dup_symbolic_name_bundle)
 target_include_directories(test_framework PRIVATE ../src)
 celix_deprecated_utils_headers(test_framework)
 
 celix_get_bundle_file(simple_test_bundle1 SIMPLE_TEST_BUNDLE1)
+celix_get_bundle_file(dup_symbolic_name_bundle DUP_SYMBOLIC_NAME_BUNDLE)
 celix_get_bundle_file(simple_test_bundle2 SIMPLE_TEST_BUNDLE2)
 celix_get_bundle_file(simple_test_bundle3 SIMPLE_TEST_BUNDLE3)
 celix_get_bundle_file(simple_test_bundle4 SIMPLE_TEST_BUNDLE4)
@@ -115,6 +118,7 @@ target_compile_definitions(test_framework PRIVATE
         SIMPLE_TEST_BUNDLE3_LOCATION="${SIMPLE_TEST_BUNDLE3}"
         SIMPLE_TEST_BUNDLE4_LOCATION="${SIMPLE_TEST_BUNDLE4_FILENAME}"
         SIMPLE_TEST_BUNDLE5_LOCATION="${SIMPLE_TEST_BUNDLE5_FILENAME}"
+        DUP_SYMBOLIC_NAME_BUNDLE_LOCATION="${DUP_SYMBOLIC_NAME_BUNDLE}"
         TEST_BUNDLE_WITH_EXCEPTION_LOCATION="${BUNDLE_WITH_EXCEPTION}"
         BUNDLE_WITH_BAD_EXPORT_LOCATION="${BUNDLE_WITH_BAD_EXPORT}"
         TEST_BUNDLE_UNRESOLVABLE_LOCATION="${UNRESOLVABLE_BUNDLE}"

libs/framework/gtest/src/CelixBundleContextBundlesTestSuite.cc

@@ -47,6 +47,7 @@ class CelixBundleContextBundlesTestSuite : public ::testing::Test {
     const char * const TEST_BND5_LOC = "" SIMPLE_TEST_BUNDLE5_LOCATION "";
     const char * const TEST_BND_WITH_EXCEPTION_LOC = "" TEST_BUNDLE_WITH_EXCEPTION_LOCATION "";
     const char * const TEST_BND_UNRESOLVABLE_LOC = "" TEST_BUNDLE_UNRESOLVABLE_LOCATION "";
+    const char * const DUP_SYMBOLIC_NAME_BUNDLE_LOC = "" DUP_SYMBOLIC_NAME_BUNDLE_LOCATION "";
 
     CelixBundleContextBundlesTestSuite() {
         properties = celix_properties_create();
@@ -79,6 +80,14 @@ TEST_F(CelixBundleContextBundlesTestSuite, InstallABundleTest) {
     ASSERT_TRUE(bndId >= 0);
 }
 
+TEST_F(CelixBundleContextBundlesTestSuite, InstallBundleWithDuplicatedSymbolicNameTest) {
+    long bndId1 = celix_bundleContext_installBundle(ctx, TEST_BND1_LOC, true);
+    ASSERT_TRUE(bndId1 >= 0);
+
+    long bndId2 = celix_bundleContext_installBundle(ctx, DUP_SYMBOLIC_NAME_BUNDLE_LOC, true);
+    ASSERT_TRUE(bndId2 < 0);
+}
+
 //TEST_F(CelixBundleContextBundlesTestSuite, InstallBundleWithBadExport) {
 //    long bndId = celix_bundleContext_installBundle(ctx, BUNDLE_WITH_BAD_EXPORT_LOCATION, true);
 //    ASSERT_TRUE(bndId >= 0);

libs/framework/src/bundle.c

@@ -79,7 +79,7 @@ celix_status_t celix_bundle_createFromArchive(celix_framework_t *framework, bund
     }
 
     *bundleOut = bundle;
-	return status;
+    return status;
 }
 
 celix_status_t bundle_destroy(bundle_pt bundle) {

libs/framework/src/framework.c

@@ -685,10 +685,13 @@ celix_framework_installBundleInternalImpl(celix_framework_t* framework, const ch
     bundle_archive_t* archive = NULL;
     celix_bundle_t* bundle = NULL;
     celix_status_t status = celix_bundleCache_createArchive(framework->cache, id, bndLoc, &archive);
-    status = CELIX_DO_IF(status, celix_bundle_createFromArchive(framework, archive, &bundle));
     if (status != CELIX_SUCCESS) {
-        celix_bundleArchive_destroy(archive);
-        fw_logCode(framework->logger, CELIX_LOG_LEVEL_ERROR, status, "Cannot install bundle without archive");
+        return status;
+    }
+    status = celix_bundle_createFromArchive(framework, archive, &bundle);
+    if (status != CELIX_SUCCESS) {
+        celix_bundleArchive_invalidate(archive);
+        celix_bundleCache_destroyArchive(framework->cache, archive);
         return status;
     }
 
@@ -783,19 +786,19 @@ celix_status_t fw_populateDependentGraph(framework_pt framework, bundle_pt expor
 }
 
 celix_status_t fw_registerService(framework_pt framework, service_registration_pt *registration, long bndId, const char* serviceName, const void* svcObj, celix_properties_t *properties) {
-	celix_status_t status = CELIX_SUCCESS;
-	char *error = NULL;
-	if (serviceName == NULL || svcObj == NULL) {
-	    status = CELIX_ILLEGAL_ARGUMENT;
-	    error = "ServiceName and SvcObj cannot be null";
-	}
+    celix_status_t status = CELIX_SUCCESS;
+    char *error = NULL;
+    if (serviceName == NULL || svcObj == NULL) {
+        status = CELIX_ILLEGAL_ARGUMENT;
+        error = "ServiceName and SvcObj cannot be null";
+    }
 
-        celix_bundle_entry_t*entry = celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(framework,
-                                                                                                          bndId);
+    celix_bundle_entry_t*entry = celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(framework,
+                                                                                               bndId);
     status = CELIX_DO_IF(status, serviceRegistry_registerService(framework->registry, entry->bnd, serviceName, svcObj, properties, registration));
     celix_bundleEntry_decreaseUseCount(entry);
     framework_logIfError(framework->logger, status, error, "Cannot register service: %s", serviceName);
-	return status;
+    return status;
 }
 
 celix_status_t fw_registerServiceFactory(framework_pt framework, service_registration_pt *registration, long bndId, const char* serviceName, service_factory_pt factory,  celix_properties_t* properties) {
@@ -1911,9 +1914,8 @@ static void celix_framework_waitForBundleEvents(celix_framework_t *fw, long bndI
 
 static long celix_framework_installAndStartBundleInternal(celix_framework_t *fw, const char *bundleLoc, bool autoStart, bool forcedAsync) {
     long bundleId = -1;
-    celix_status_t status = CELIX_SUCCESS;
-
-    if (celix_framework_installBundleInternal(fw, bundleLoc, &bundleId) == CELIX_SUCCESS) {
+    celix_status_t status = celix_framework_installBundleInternal(fw, bundleLoc, &bundleId) ;
+    if (status == CELIX_SUCCESS) {
         if (autoStart) {
             celix_bundle_entry_t* bndEntry = celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(fw,
                                                                                                                      bundleId);
@@ -2375,15 +2377,10 @@ celix_status_t celix_framework_updateBundleEntry(celix_framework_t* framework,
     if (updatedBundleUrl == NULL) {
         updatedBundleUrl = location;
     } else if (strcmp(location, updatedBundleUrl) != 0) {
-        long existingBndId;
-        status = celix_bundleCache_findBundleIdForLocation(framework->cache, updatedBundleUrl, &existingBndId);
-        if (status != CELIX_SUCCESS) {
-            fw_log(framework->logger,
-                   CELIX_LOG_LEVEL_ERROR,
-                   "Could not read bundle cache to check if bundle location %s exists in cache. Update failed.",
-                   updatedBundleUrl);
-            return status;
-        } else if (existingBndId != -1 && existingBndId != bundleId) {
+        long existingBndId = -1L;
+        // celix_bundleCache_findBundleIdForLocation can never fail since there is at lease bndEntry is installed
+        (void)celix_bundleCache_findBundleIdForLocation(framework->cache, updatedBundleUrl, &existingBndId);
+        if (existingBndId != -1 && existingBndId != bundleId) {
             fw_log(framework->logger,
                    CELIX_LOG_LEVEL_ERROR,
                    "Specified bundle location %s exists in cache with a different id. Update failed.",
@@ -2417,14 +2414,14 @@ celix_status_t celix_framework_updateBundleEntry(celix_framework_t* framework,
 
     celix_bundle_entry_t* entry =
         celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(framework, bundleId);
+    celix_auto(celix_bundle_entry_use_guard_t) entryGuard = celix_bundleEntryUseGuard_init(entry);
     celixMutexLockGuard_deinit(&lck);
     status = celix_framework_startBundleEntry(framework, entry);
-    celix_bundleEntry_decreaseUseCount(entry);
     if (status != CELIX_SUCCESS) {
         fw_log(framework->logger,
                CELIX_LOG_LEVEL_ERROR,
                "Failed to start updated bundle %s",
-               celix_bundle_getSymbolicName(bndEntry->bnd));
+               celix_bundle_getSymbolicName(entry->bnd));
         return CELIX_BUNDLE_EXCEPTION;
     }
 

libs/framework/src/framework_private.h

@@ -376,8 +376,8 @@ static CELIX_UNUSED inline celix_bundle_entry_use_guard_t celix_bundleEntryUseGu
 static CELIX_UNUSED inline void celix_bundleEntryUseGuard_deinit(celix_bundle_entry_use_guard_t* guard) {
     if (guard->entry) {
         celix_bundleEntry_decreaseUseCount(guard->entry);
+        guard->entry = NULL;
     }
-    guard->entry = NULL;
 }
 
 CELIX_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(celix_bundle_entry_use_guard_t, celix_bundleEntryUseGuard_deinit)