Merge pull request #596 from apache/hotfix/595-use-after-free-caused-…

…by-cancelled-tracker

hotfix/fix use-after-free caused by cancelled service tracker.

bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.c

@@ -153,8 +153,7 @@ celix_status_t discoveryZeroconfWatcher_create(celix_bundle_context_t *ctx, celi
     *watcherOut = watcher;
     return CELIX_SUCCESS;
 thread_err:
-    celix_bundleContext_stopTrackerAsync(ctx, watcher->epListenerTrkId, NULL, NULL);
-    celix_bundleContext_waitForAsyncStopTracker(ctx, watcher->epListenerTrkId);
+    celix_bundleContext_stopTracker(ctx, watcher->epListenerTrkId);
 epl_tracker_err:
     celix_longHashMap_destroy(watcher->epls);
     celix_stringHashMap_destroy(watcher->watchedServices);

libs/framework/include_deprecated/service_tracker.h

@@ -83,6 +83,15 @@ CELIX_FRAMEWORK_DEPRECATED_EXPORT celix_service_tracker_t* celix_serviceTracker_
 
 );
 
+/**
+ * Creates a closed service tracker.
+ * It is similar to serviceTracker_create.
+ */
+CELIX_FRAMEWORK_DEPRECATED_EXPORT celix_service_tracker_t* celix_serviceTracker_createClosedWithOptions(
+    celix_bundle_context_t *ctx,
+    const celix_service_tracking_options_t *opts
+);
+
 /**
  * Creates and starts (open) a service tracker.
  * Note that is different from the serviceTracker_create function, because is also starts the service tracker

libs/framework/src/bundle_context.c

@@ -1235,25 +1235,20 @@ static void celix_bundleContext_createTrackerOnEventLoop(void *data) {
     assert(celix_framework_isCurrentThreadTheEventLoop(entry->ctx->framework));
     celixThreadMutex_lock(&entry->ctx->mutex);
     bool cancelled = entry->cancelled;
-    celixThreadMutex_unlock(&entry->ctx->mutex);
     if (cancelled) {
         fw_log(entry->ctx->framework->logger, CELIX_LOG_LEVEL_DEBUG, "Creating of service tracker was cancelled. trk id = %li, svc name tracked = %s", entry->trackerId, entry->opts.filter.serviceName);
+        celixThreadMutex_unlock(&entry->ctx->mutex);
+        return;
+    }
+    celix_service_tracker_t *tracker = celix_serviceTracker_createClosedWithOptions(entry->ctx, &entry->opts);
+    if (tracker == NULL) {
+        fw_log(entry->ctx->framework->logger, CELIX_LOG_LEVEL_ERROR, "Cannot create tracker for bnd %s (%li)", celix_bundle_getSymbolicName(entry->ctx->bundle), celix_bundle_getId(entry->ctx->bundle));
     } else {
-        celix_service_tracker_t *tracker = celix_serviceTracker_createWithOptions(entry->ctx, &entry->opts);
-        if (tracker != NULL) {
-            celixThreadMutex_lock(&entry->ctx->mutex);
-            // double-check is necessary to eliminate first-check-then-do race condition
-            if(!entry->cancelled) {
-                entry->tracker = tracker;
-            }
-            celixThreadMutex_unlock(&entry->ctx->mutex);
-            if(entry->tracker == NULL) {
-                assert(entry->cancelled);
-                celix_serviceTracker_destroy(tracker);
-            }
-        } else {
-            fw_log(entry->ctx->framework->logger, CELIX_LOG_LEVEL_ERROR, "Cannot create tracker for bnd %s (%li)", celix_bundle_getSymbolicName(entry->ctx->bundle), celix_bundle_getId(entry->ctx->bundle));
-        }
+        entry->tracker = tracker;
+    }
+    celixThreadMutex_unlock(&entry->ctx->mutex);
+    if (tracker) {
+        serviceTracker_open(tracker);
     }
 }
 

libs/framework/src/service_tracker.c

@@ -632,36 +632,43 @@ celix_service_tracker_t* celix_serviceTracker_create(
     return celix_serviceTracker_createWithOptions(ctx, &opts);
 }
 
-celix_service_tracker_t* celix_serviceTracker_createWithOptions(
-        bundle_context_t *ctx,
-        const celix_service_tracking_options_t *opts
-) {
-    celix_service_tracker_t *tracker = NULL;
+celix_service_tracker_t* celix_serviceTracker_createClosedWithOptions(celix_bundle_context_t* ctx,
+                                                                      const celix_service_tracking_options_t* opts) {
+    celix_service_tracker_t* tracker = NULL;
     const char* serviceName = NULL;
-    char *filter = NULL;
+    char* filter = NULL;
     assert(ctx != NULL);
     assert(opts != NULL);
     serviceName = opts->filter.serviceName == NULL ? "*" : opts->filter.serviceName;
-    filter = celix_serviceRegistry_createFilterFor(ctx->framework->registry, opts->filter.serviceName, opts->filter.versionRange, opts->filter.filter);
-    if(filter == NULL) {
-        celix_framework_log(ctx->framework->logger, CELIX_LOG_LEVEL_ERROR, __FUNCTION__, __BASE_FILE__, __LINE__,
+    filter = celix_serviceRegistry_createFilterFor(
+        ctx->framework->registry, opts->filter.serviceName, opts->filter.versionRange, opts->filter.filter);
+    if (filter == NULL) {
+        celix_framework_log(ctx->framework->logger,
+                            CELIX_LOG_LEVEL_ERROR,
+                            __FUNCTION__,
+                            __BASE_FILE__,
+                            __LINE__,
                             "Error cannot create filter.");
         return NULL;
     }
     tracker = calloc(1, sizeof(*tracker));
-    if(tracker == NULL) {
+    if (tracker == NULL) {
         free(filter);
-        celix_framework_log(ctx->framework->logger, CELIX_LOG_LEVEL_ERROR, __FUNCTION__, __BASE_FILE__, __LINE__,
+        celix_framework_log(ctx->framework->logger,
+                            CELIX_LOG_LEVEL_ERROR,
+                            __FUNCTION__,
+                            __BASE_FILE__,
+                            __LINE__,
                             "No memory for tracker.");
-       return NULL;
+        return NULL;
     }
 
     tracker->context = ctx;
     tracker->serviceName = celix_utils_strdup(serviceName);
     tracker->filter = filter;
     tracker->state = CELIX_SERVICE_TRACKER_CLOSED;
 
-    //setting callbacks
+    // setting callbacks
     tracker->callbackHandle = opts->callbackHandle;
     tracker->set = opts->set;
     tracker->add = opts->add;
@@ -684,8 +691,18 @@ celix_service_tracker_t* celix_serviceTracker_createWithOptions(
     tracker->currentHighestServiceId = -1;
 
     tracker->listener.handle = tracker;
-    tracker->listener.serviceChanged = (void *) serviceTracker_serviceChanged;
+    tracker->listener.serviceChanged = (void*)serviceTracker_serviceChanged;
+    return tracker;
+}
 
+celix_service_tracker_t* celix_serviceTracker_createWithOptions(
+        bundle_context_t *ctx,
+        const celix_service_tracking_options_t *opts
+) {
+    celix_service_tracker_t *tracker = celix_serviceTracker_createClosedWithOptions(ctx, opts);
+    if(tracker == NULL) {
+       return NULL;
+    }
     serviceTracker_open(tracker);
     return tracker;
 }