Merge pull request atomvm#1603 from pguyot/w13/fix-resource-monitor-concurrency-issue

Use ref count and lock on list of monitors for a resource type to ensure that
resource still exists when calling the monitor handler.

Should fix atomvm#1589

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Author: bettio

src/libAtomVM/context.c

@@ -180,8 +180,7 @@ void context_destroy(Context *ctx)
             struct Monitor *monitor = GET_LIST_ENTRY(item, struct Monitor, monitor_list_head);
             void *resource = term_to_term_ptr(monitor->monitor_obj);
             struct RefcBinary *refc = refc_binary_from_data(resource);
-            resource_type_demonitor(refc->resource_type, monitor->ref_ticks);
-            refc->resource_type->down(&env, resource, &ctx->process_id, &monitor->ref_ticks);
+            resource_type_fire_monitor(refc->resource_type, &env, resource, ctx->process_id, monitor->ref_ticks);
             free(monitor);
         }
     }

src/libAtomVM/refc_binary.c

@@ -66,6 +66,20 @@ void refc_binary_increment_refcount(struct RefcBinary *refc)
 bool refc_binary_decrement_refcount(struct RefcBinary *refc, struct GlobalContext *global)
 {
     if (--refc->ref_count == 0) {
+        if (refc->resource_type && refc->resource_type->down) {
+            // There may be monitors associated with this resource.
+            destroy_resource_monitors(refc, global);
+            // After this point, the resource can no longer be found by
+            // resource_type_fire_monitor
+            // However, resource_type_fire_monitor may have incremented ref_count
+            // to call the monitor handler.
+            // So we check ref_count again. We're not affected by the ABA problem
+            // here as the resource cannot (should not) be monitoring while it is
+            // being destroyed, i.e. no resource monitor will be created now
+            if (UNLIKELY(refc->ref_count != 0)) {
+                return false;
+            }
+        }
         synclist_remove(&global->refc_binaries, &refc->head);
         refc_binary_destroy(refc, global);
         return true;
@@ -78,10 +92,6 @@ void refc_binary_destroy(struct RefcBinary *refc, struct GlobalContext *global)
     UNUSED(global);
 
     if (refc->resource_type) {
-        if (refc->resource_type->down) {
-            // There may be monitors associated with this resource.
-            destroy_resource_monitors(refc, global);
-        }
         if (refc->resource_type->dtor) {
             ErlNifEnv env;
             erl_nif_env_partial_init_from_globalcontext(&env, global);

src/libAtomVM/resources.c

@@ -360,6 +360,31 @@ int enif_monitor_process(ErlNifEnv *env, void *obj, const ErlNifPid *target_pid,
     return 0;
 }
 
+void resource_type_fire_monitor(struct ResourceType *resource_type, ErlNifEnv *env, void *resource, int32_t process_id, uint64_t ref_ticks)
+{
+    struct RefcBinary *refc = NULL;
+    struct ListHead *monitors = synclist_wrlock(&resource_type->monitors);
+    struct ListHead *item;
+    LIST_FOR_EACH (item, monitors) {
+        struct ResourceMonitor *monitor = GET_LIST_ENTRY(item, struct ResourceMonitor, resource_list_head);
+        if (monitor->ref_ticks == ref_ticks) {
+            // Resource still exists.
+            refc = refc_binary_from_data(resource);
+            refc_binary_increment_refcount(refc);
+            list_remove(&monitor->resource_list_head);
+            free(monitor);
+            break;
+        }
+    }
+
+    synclist_unlock(&resource_type->monitors);
+
+    if (refc) {
+        resource_type->down(env, resource, &process_id, &ref_ticks);
+        refc_binary_decrement_refcount(refc, env->global);
+    }
+}
+
 void resource_type_demonitor(struct ResourceType *resource_type, uint64_t ref_ticks)
 {
     struct ListHead *monitors = synclist_wrlock(&resource_type->monitors);

src/libAtomVM/resources.h

@@ -149,6 +149,19 @@ void destroy_resource_monitors(struct RefcBinary *resource, GlobalContext *globa
  */
 term select_event_make_notification(void *rsrc_obj, uint64_t ref_ticks, bool is_write, Heap *heap);
 
+/**
+ * @brief Call down handler for a given resource and remove monitor from list.
+ * @details handler is called while holding lock on the list of monitors and
+ * if monitor is still in the list of resource monitors, thus ensuring that
+ * the resource still exists.
+ * @param resource_type type holding the list of monitors
+ * @param env environment for calling the down handler
+ * @param resource resource that monitored the process
+ * @param process_id id of the process monitored
+ * @param ref_ticks reference of the monitor
+ */
+void resource_type_fire_monitor(struct ResourceType *resource_type, ErlNifEnv *env, void *resource, int32_t process_id, uint64_t ref_ticks);
+
 /**
  * @brief Remove monitor from list of monitors.
  * @param resource_type type holding the list of monitors