Merge pull request #1604 from pguyot/w13/fix-erlnif-monitor-possible-crash

Fix possible crash with enif_demonitor_process

Resource may no longer exist when enif_demonitor_process is called if the
monitor was already destroyed as the resource was deallocated. Fix this by
storing resource_type in ErlNifMonitor.

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/erl_nif.h

@@ -56,7 +56,11 @@ typedef struct ResourceType ErlNifResourceType;
 /**
  * @brief Opaque monitor type
  */
-typedef uint64_t ErlNifMonitor;
+typedef struct
+{
+    struct ResourceType *resource_type;
+    uint64_t ref_ticks;
+} ErlNifMonitor;
 
 /**
  * @brief Selectable event.

src/libAtomVM/resources.c

@@ -354,7 +354,8 @@ int enif_monitor_process(ErlNifEnv *env, void *obj, const ErlNifPid *target_pid,
     globalcontext_get_process_unlock(env->global, target);
 
     if (mon) {
-        *mon = ref_ticks;
+        mon->ref_ticks = ref_ticks;
+        mon->resource_type = resource_type;
     }
 
     return 0;
@@ -380,7 +381,10 @@ void resource_type_fire_monitor(struct ResourceType *resource_type, ErlNifEnv *e
     synclist_unlock(&resource_type->monitors);
 
     if (refc) {
-        resource_type->down(env, resource, &process_id, &ref_ticks);
+        ErlNifMonitor monitor;
+        monitor.ref_ticks = ref_ticks;
+        monitor.resource_type = resource_type;
+        resource_type->down(env, resource, &process_id, &monitor);
         refc_binary_decrement_refcount(refc, env->global);
     }
 }
@@ -404,17 +408,21 @@ void resource_type_demonitor(struct ResourceType *resource_type, uint64_t ref_ti
 int enif_demonitor_process(ErlNifEnv *env, void *obj, const ErlNifMonitor *mon)
 {
     GlobalContext *global = env->global;
-    struct RefcBinary *resource = refc_binary_from_data(obj);
-    struct ResourceType *resource_type = resource->resource_type;
-    if (resource_type == NULL || resource_type->down == NULL) {
+    struct ResourceType *resource_type = mon->resource_type;
+    if (resource_type->down == NULL) {
         return -1;
     }
 
     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 == *mon) {
+        if (monitor->ref_ticks == mon->ref_ticks) {
+            struct RefcBinary *resource = refc_binary_from_data(obj);
+            if (resource->resource_type != mon->resource_type) {
+                return -1;
+            }
+
             Context *target = globalcontext_get_process_lock(global, monitor->process_id);
             if (target) {
                 mailbox_send_ref_signal(target, DemonitorSignal, monitor->ref_ticks);
@@ -457,8 +465,8 @@ void destroy_resource_monitors(struct RefcBinary *resource, GlobalContext *globa
 
 int enif_compare_monitors(const ErlNifMonitor *monitor1, const ErlNifMonitor *monitor2)
 {
-    uint64_t ref_ticks1 = *monitor1;
-    uint64_t ref_ticks2 = *monitor2;
+    uint64_t ref_ticks1 = monitor1->ref_ticks;
+    uint64_t ref_ticks2 = monitor2->ref_ticks;
     if (ref_ticks1 < ref_ticks2) {
         return -1;
     }

tests/test-enif.c

@@ -30,11 +30,11 @@
 
 static uint32_t cb_read_resource = 0;
 static int32_t down_pid = 0;
-static ErlNifMonitor down_mon = 0;
+static ErlNifMonitor down_mon = { NULL, 0 };
 
 static uint32_t cb_read_resource_two = 0;
 static int32_t down_pid_two = 0;
-static ErlNifMonitor down_mon_two = 0;
+static ErlNifMonitor down_mon_two = { NULL, 0 };
 
 static int32_t lockable_pid = 0;
 
@@ -233,7 +233,7 @@ void test_resource_monitor()
     // Monitor called on destroy
     cb_read_resource = 0;
     down_pid = 0;
-    down_mon = 0;
+    down_mon.ref_ticks = 0;
     ctx = context_new(glb);
     pid = ctx->process_id;
     monitor_result = enif_monitor_process(&env, ptr, &pid, &mon);
@@ -248,7 +248,7 @@ void test_resource_monitor()
     // Monitor not called if demonitored
     cb_read_resource = 0;
     down_pid = 0;
-    down_mon = 0;
+    down_mon.ref_ticks = 0;
     ctx = context_new(glb);
     pid = ctx->process_id;
     monitor_result = enif_monitor_process(&env, ptr, &pid, &mon);
@@ -265,7 +265,7 @@ void test_resource_monitor()
     // Monitor not called if resource is deallocated
     cb_read_resource = 0;
     down_pid = 0;
-    down_mon = 0;
+    down_mon.ref_ticks = 0;
     ctx = context_new(glb);
     pid = ctx->process_id;
     monitor_result = enif_monitor_process(&env, ptr, &pid, &mon);
@@ -317,7 +317,7 @@ void test_resource_monitor_handler_can_lock()
     // Monitor called on destroy
     cb_read_resource = 0;
     down_pid = 0;
-    down_mon = 0;
+    down_mon.ref_ticks = 0;
     ctx = context_new(glb);
     another_ctx = context_new(glb);
     lockable_pid = another_ctx->process_id;
@@ -381,8 +381,8 @@ void test_resource_monitor_two_resources_two_processes()
     cb_read_resource = 0;
     down_pid = 0;
     down_pid_two = 0;
-    down_mon = 0;
-    down_mon_two = 0;
+    down_mon.ref_ticks = 0;
+    down_mon_two.ref_ticks = 0;
     ctx_1 = context_new(glb);
     ctx_2 = context_new(glb);
     pid_1 = ctx_1->process_id;
@@ -411,7 +411,7 @@ void test_resource_monitor_two_resources_two_processes()
     cb_read_resource = 0;
     cb_read_resource_two = 0;
     down_pid = 0;
-    down_mon = 0;
+    down_mon.ref_ticks = 0;
 
     // Process #2 terminates, mon_3 is fired.
     scheduler_terminate(ctx_2);