Merge pull request #1498 from pguyot/w04/fix-refcount-calculation

Fix refcount calculation when copying data to heaps

Ref count should be incremented whenever the ref counted binary is added to
the mso list and decremented when the mso list is swept.

Also refc_binaries are allocated with a ref count of 0 until they are made
a term, with the exception of enif_alloc_resource that should be balanced
by an enif_release_resource following OTP semantic.

Also add a new test for heap operations including gc.

Also fix race conditions with demonitor in both otp_socket and emscripten
code that yielded a crash by over decrementing ref count.

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

.github/workflows/build-and-test-macos.yaml

@@ -95,6 +95,11 @@ jobs:
       run: |
         ./tests/test-enif
 
+    - name: "Test: test-heap"
+      working-directory: build
+      run: |
+        ./tests/test-heap
+
     - name: "Test: test-mailbox"
       working-directory: build
       run: |

.github/workflows/build-and-test-on-freebsd.yaml

@@ -104,6 +104,11 @@ jobs:
           echo "%%"
           ./tests/test-enif
 
+          echo "%%"
+          echo "%% Running test-heap ..."
+          echo "%%"
+          ./tests/test-heap
+
           echo "%%"
           echo "%% Running test-mailbox ..."
           echo "%%"

.github/workflows/build-and-test-other.yaml

@@ -169,12 +169,15 @@ jobs:
         make AtomVM &&
         make test-erlang &&
         make test-enif &&
+        make test-heap &&
         make test-mailbox &&
         make test-structs &&
         file ./tests/test-erlang &&
         ./tests/test-erlang -s prime_smp &&
         file ./tests/test-enif &&
         ./tests/test-enif &&
+        file ./tests/test-heap &&
+        ./tests/test-heap &&
         file ./tests/test-mailbox &&
         ./tests/test-mailbox &&
         file ./tests/test-structs &&

.github/workflows/build-and-test.yaml

@@ -342,6 +342,13 @@ jobs:
         ./tests/test-enif
         valgrind ./tests/test-enif
 
+    - name: "Test: test-heap"
+      working-directory: build
+      run: |
+        ulimit -c unlimited
+        ./tests/test-heap
+        valgrind ./tests/test-heap
+
     - name: "Test: test-mailbox"
       working-directory: build
       run: |

.github/workflows/build-linux-artifacts.yaml

@@ -226,12 +226,15 @@ jobs:
         VERBOSE=1 make AtomVM &&
         make test-erlang &&
         make test-enif &&
+        make test-heap &&
         make test-mailbox &&
         make test-structs &&
         file ./tests/test-erlang &&
         ./tests/test-erlang -s prime_smp &&
         file ./tests/test-enif &&
         ./tests/test-enif &&
+        file ./tests/test-heap &&
+        ./tests/test-heap &&
         file ./tests/test-mailbox &&
         ./tests/test-mailbox &&
         file ./tests/test-structs &&

CHANGELOG.md

@@ -47,6 +47,8 @@ bug when handling errors from BIFs used as NIFs (when called with `CALL_EXT` and
 - Fixed matching of binaries on unaligned boundaries for code compiled with older versions of OTP
 - Added missing out of memory handling in binary_to_atom
 - Fixed call to funs such as fun erlang:'not'/1, that make use of BIFs
+- Fixed potential crashes or memory leaks caused by a mistake in calculation of reference counts
+and a race condition in otp_socket code
 
 ## [0.6.5] - 2024-10-15
 

src/libAtomVM/erl_nif_priv.h

@@ -23,6 +23,7 @@
 
 #include "context.h"
 #include "memory.h"
+#include "resources.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -67,6 +68,19 @@ static inline void erl_nif_env_partial_init_from_globalcontext(ErlNifEnv *env, G
     env->x[1] = term_nil();
 }
 
+static inline void erl_nif_env_partial_init_from_resource(ErlNifEnv *env, void *resource)
+{
+    struct RefcBinary *refc = refc_binary_from_data(resource);
+    env->global = refc->resource_type->global;
+    env->heap.root = NULL;
+    env->heap.heap_start = NULL;
+    env->heap.heap_ptr = NULL;
+    env->heap.heap_end = NULL;
+    env->stack_pointer = NULL;
+    env->x[0] = term_nil();
+    env->x[1] = term_nil();
+}
+
 #ifdef __cplusplus
 }
 #endif

src/libAtomVM/memory.c

@@ -99,7 +99,7 @@ static inline enum MemoryGCResult memory_heap_alloc_new_fragment(Heap *heap, siz
     term *old_end = heap->heap_end;
     term mso_list = root_fragment->mso_list;
     if (UNLIKELY(memory_init_heap(heap, size) != MEMORY_GC_OK)) {
-        TRACE("Unable to allocate memory fragment.  size=%u\n", size);
+        TRACE("Unable to allocate memory fragment.  size=%u\n", (unsigned int) size);
         return MEMORY_GC_ERROR_FAILED_ALLOCATION;
     }
     // Convert root fragment to non-root fragment.
@@ -123,7 +123,7 @@ enum MemoryGCResult memory_erl_nif_env_ensure_free(ErlNifEnv *env, size_t size)
             }
         } else {
             if (UNLIKELY(memory_init_heap(&env->heap, size) != MEMORY_GC_OK)) {
-                TRACE("Unable to allocate memory fragment.  size=%u\n", size);
+                TRACE("Unable to allocate memory fragment.  size=%u\n", (unsigned int) size);
                 return MEMORY_GC_ERROR_FAILED_ALLOCATION;
             }
         }
@@ -203,44 +203,42 @@ enum MemoryGCResult memory_ensure_free_with_roots(Context *c, size_t size, size_
             if (UNLIKELY(c->has_max_heap_size && (target_size > c->max_heap_size))) {
                 return MEMORY_GC_DENIED_ALLOCATION;
             }
-            if (target_size != memory_size) {
-                if (UNLIKELY(memory_gc(c, target_size, num_roots, roots) != MEMORY_GC_OK)) {
-                    // TODO: handle this more gracefully
-                    TRACE("Unable to allocate memory for GC.  target_size=%zu\n", target_size);
-                    return MEMORY_GC_ERROR_FAILED_ALLOCATION;
-                }
-                should_gc = alloc_mode == MEMORY_FORCE_SHRINK;
-                size_t new_memory_size = memory_heap_memory_size(&c->heap);
-                size_t new_target_size = new_memory_size;
-                size_t new_free_space = context_avail_free_memory(c);
-                switch (c->heap_growth_strategy) {
-                    case BoundedFreeHeapGrowth: {
-                        size_t maximum_free_space = 2 * (size + MIN_FREE_SPACE_SIZE);
-                        should_gc = should_gc || (alloc_mode != MEMORY_NO_SHRINK && new_free_space > maximum_free_space);
-                        if (should_gc) {
-                            new_target_size = (new_memory_size - new_free_space) + maximum_free_space;
-                        }
-                    } break;
-                    case MinimumHeapGrowth:
-                        should_gc = should_gc || (alloc_mode != MEMORY_NO_SHRINK && new_free_space > 0);
-                        if (should_gc) {
-                            new_target_size = new_memory_size - new_free_space + size;
-                        }
-                        break;
-                    case FibonacciHeapGrowth:
-                        should_gc = should_gc || (new_memory_size > FIBONACCI_HEAP_GROWTH_REDUCTION_THRESHOLD && new_free_space >= 3 * new_memory_size / 4);
-                        if (should_gc) {
-                            new_target_size = next_fibonacci_heap_size(new_memory_size - new_free_space + size);
-                        }
-                        break;
-                }
-                if (should_gc) {
-                    new_target_size = MAX(c->has_min_heap_size ? c->min_heap_size : 0, new_target_size);
-                    if (new_target_size != new_memory_size) {
-                        if (UNLIKELY(MEMORY_SHRINK(c, new_target_size, num_roots, roots) != MEMORY_GC_OK)) {
-                            TRACE("Unable to allocate memory for GC shrink.  new_memory_size=%zu new_free_space=%zu size=%u\n", new_memory_size, new_free_space, size);
-                            return MEMORY_GC_ERROR_FAILED_ALLOCATION;
-                        }
+            if (UNLIKELY(memory_gc(c, target_size, num_roots, roots) != MEMORY_GC_OK)) {
+                // TODO: handle this more gracefully
+                TRACE("Unable to allocate memory for GC.  target_size=%zu\n", target_size);
+                return MEMORY_GC_ERROR_FAILED_ALLOCATION;
+            }
+            should_gc = alloc_mode == MEMORY_FORCE_SHRINK;
+            size_t new_memory_size = memory_heap_memory_size(&c->heap);
+            size_t new_target_size = new_memory_size;
+            size_t new_free_space = context_avail_free_memory(c);
+            switch (c->heap_growth_strategy) {
+                case BoundedFreeHeapGrowth: {
+                    size_t maximum_free_space = 2 * (size + MIN_FREE_SPACE_SIZE);
+                    should_gc = should_gc || (alloc_mode != MEMORY_NO_SHRINK && new_free_space > maximum_free_space);
+                    if (should_gc) {
+                        new_target_size = (new_memory_size - new_free_space) + maximum_free_space;
+                    }
+                } break;
+                case MinimumHeapGrowth:
+                    should_gc = should_gc || (alloc_mode != MEMORY_NO_SHRINK && new_free_space > 0);
+                    if (should_gc) {
+                        new_target_size = new_memory_size - new_free_space + size;
+                    }
+                    break;
+                case FibonacciHeapGrowth:
+                    should_gc = should_gc || (new_memory_size > FIBONACCI_HEAP_GROWTH_REDUCTION_THRESHOLD && new_free_space >= 3 * new_memory_size / 4);
+                    if (should_gc) {
+                        new_target_size = next_fibonacci_heap_size(new_memory_size - new_free_space + size);
+                    }
+                    break;
+            }
+            if (should_gc) {
+                new_target_size = MAX(c->has_min_heap_size ? c->min_heap_size : 0, new_target_size);
+                if (new_target_size != new_memory_size) {
+                    if (UNLIKELY(MEMORY_SHRINK(c, new_target_size, num_roots, roots) != MEMORY_GC_OK)) {
+                        TRACE("Unable to allocate memory for GC shrink.  new_memory_size=%zu new_free_space=%zu size=%u\n", new_memory_size, new_free_space, (unsigned int) size);
+                        return MEMORY_GC_ERROR_FAILED_ALLOCATION;
                     }
                 }
             }
@@ -301,7 +299,7 @@ static enum MemoryGCResult memory_gc(Context *ctx, size_t new_size, size_t num_r
 
     TRACE("- Running copy GC on provided roots\n");
     for (size_t i = 0; i < num_roots; i++) {
-        roots[i] = memory_shallow_copy_term(old_root_fragment, roots[i], &ctx->heap.heap_ptr, 1);
+        roots[i] = memory_shallow_copy_term(old_root_fragment, roots[i], &ctx->heap.heap_ptr, true);
     }
 
     term *temp_start = new_heap;
@@ -641,6 +639,7 @@ static void memory_scan_and_copy(HeapFragment *old_fragment, term *mem_start, co
                     term ref = ((term) ptr) | TERM_BOXED_VALUE_TAG;
                     if (!term_refc_binary_is_const(ref)) {
                         *mso_list = term_list_init_prepend(ptr + REFC_BINARY_CONS_OFFSET, ref, *mso_list);
+                        refc_binary_increment_refcount((struct RefcBinary *) term_refc_binary_ptr(ref));
                     }
                     break;
                 }
@@ -851,10 +850,6 @@ HOT_FUNC static term memory_shallow_copy_term(HeapFragment *old_fragment, term t
 
         if (move) {
             memory_replace_with_moved_marker(boxed_value, new_term);
-        } else if (term_is_refc_binary(t)) { // copy, not a move; increment refcount
-            if (!term_refc_binary_is_const(t)) {
-                refc_binary_increment_refcount((struct RefcBinary *) term_refc_binary_ptr(t));
-            }
         }
 
         return new_term;
@@ -930,15 +925,16 @@ void memory_sweep_mso_list(term mso_list, GlobalContext *global, bool from_task)
         TERM_DEBUG_ASSERT(term_is_boxed(h))
         term *boxed_value = term_to_term_ptr(h);
         if (memory_is_moved_marker(boxed_value)) {
-            // it has been moved, so it is referenced
-        } else if (term_is_refc_binary(h) && !term_refc_binary_is_const(h)) {
+            h = memory_dereference_moved_marker(boxed_value);
+        }
+        if (term_is_refc_binary(h) && !term_refc_binary_is_const(h)) {
             // unreferenced binary; decrement reference count
 #ifdef AVM_TASK_DRIVER_ENABLED
             if (from_task) {
-                globalcontext_refc_decrement_refcount_from_task(global, (struct RefcBinary *) term_refc_binary_ptr(h));
+                globalcontext_refc_decrement_refcount_from_task(global, term_refc_binary_ptr(h));
             } else {
 #endif
-                refc_binary_decrement_refcount((struct RefcBinary *) term_refc_binary_ptr(h), global);
+                refc_binary_decrement_refcount(term_refc_binary_ptr(h), global);
 #ifdef AVM_TASK_DRIVER_ENABLED
             }
 #endif

src/libAtomVM/otp_socket.c

@@ -284,10 +284,11 @@ static void socket_stop(ErlNifEnv *caller_env, void *obj, ErlNifEvent event, int
     struct SocketResource *rsrc_obj = (struct SocketResource *) obj;
 
     if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
-        enif_demonitor_process(caller_env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
+        if (LIKELY(enif_demonitor_process(caller_env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) {
+            struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
+            refc_binary_decrement_refcount(rsrc_refc, caller_env->global);
+        }
         rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
-        struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
-        refc_binary_decrement_refcount(rsrc_refc, caller_env->global);
     }
 
     TRACE("socket_stop called on fd=%i\n", rsrc_obj->fd);
@@ -958,10 +959,11 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
     ErlNifEnv *env = erl_nif_env_from_context(ctx);
     if (rsrc_obj->selecting_process_id != ctx->process_id && rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
         // demonitor can fail if process is gone.
-        enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
+        if (LIKELY(enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) {
+            // decrement ref count as we are demonitoring
+            refc_binary_decrement_refcount(rsrc_refc, ctx->global);
+        }
         rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
-        // decrement ref count as we are demonitoring
-        refc_binary_decrement_refcount(rsrc_refc, ctx->global);
     }
     // Monitor first as select is less likely to fail and it's less expensive to demonitor
     // if select fails than to stop select if monitor fails
@@ -985,10 +987,11 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
         send_closed_notification(ctx, argv[0], ctx->process_id, rsrc_obj);
     } else {
         if (UNLIKELY(enif_select(erl_nif_env_from_context(ctx), rsrc_obj->fd, ERL_NIF_SELECT_READ, rsrc_obj, &ctx->process_id, select_ref_term) < 0)) {
-            enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
+            if (LIKELY(enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) {
+                refc_binary_decrement_refcount(rsrc_refc, ctx->global);
+            }
             rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
             SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);
-            refc_binary_decrement_refcount(rsrc_refc, ctx->global);
             RAISE_ERROR(BADARG_ATOM);
         }
     }
@@ -1032,10 +1035,11 @@ static term nif_socket_select_read(Context *ctx, int argc, term argv[])
             // noop
             break;
         default:
-            enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor);
+            if (LIKELY(enif_demonitor_process(env, rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) {
+                refc_binary_decrement_refcount(rsrc_refc, ctx->global);
+            }
             LWIP_END();
             SMP_RWLOCK_UNLOCK(rsrc_obj->socket_lock);
-            refc_binary_decrement_refcount(rsrc_refc, ctx->global);
             RAISE_ERROR(BADARG_ATOM);
     }
     LWIP_END();
@@ -1059,10 +1063,11 @@ static term nif_socket_select_stop(Context *ctx, int argc, term argv[])
     // Avoid the race condition with select object here.
     SMP_RWLOCK_WRLOCK(rsrc_obj->socket_lock);
     if (rsrc_obj->selecting_process_id != INVALID_PROCESS_ID) {
-        enif_demonitor_process(erl_nif_env_from_context(ctx), rsrc_obj, &rsrc_obj->selecting_process_monitor);
+        if (LIKELY(enif_demonitor_process(erl_nif_env_from_context(ctx), rsrc_obj, &rsrc_obj->selecting_process_monitor) == 0)) {
+            struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
+            refc_binary_decrement_refcount(rsrc_refc, ctx->global);
+        }
         rsrc_obj->selecting_process_id = INVALID_PROCESS_ID;
-        struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
-        refc_binary_decrement_refcount(rsrc_refc, ctx->global);
     }
 #if OTP_SOCKET_BSD
     if (UNLIKELY(enif_select(erl_nif_env_from_context(ctx), rsrc_obj->fd, ERL_NIF_SELECT_STOP, rsrc_obj, NULL, term_nil()) < 0)) {
@@ -1555,13 +1560,14 @@ static term nif_socket_listen(Context *ctx, int argc, term argv[])
 //
 
 #if OTP_SOCKET_LWIP
-static term make_accepted_socket_term(struct SocketResource *conn_rsrc_obj, Heap *heap, GlobalContext *global)
+static term make_accepted_socket_term(Context *ctx, struct SocketResource *conn_rsrc_obj)
 {
-    term obj = term_from_resource(conn_rsrc_obj, heap);
+    term obj = enif_make_resource(erl_nif_env_from_context(ctx), conn_rsrc_obj);
+    enif_release_resource(conn_rsrc_obj);
 
-    term socket_term = term_alloc_tuple(2, heap);
-    uint64_t ref_ticks = globalcontext_get_ref_ticks(global);
-    term ref = term_from_ref_ticks(ref_ticks, heap);
+    term socket_term = term_alloc_tuple(2, &ctx->heap);
+    uint64_t ref_ticks = globalcontext_get_ref_ticks(ctx->global);
+    term ref = term_from_ref_ticks(ref_ticks, &ctx->heap);
     term_put_tuple_element(socket_term, 0, obj);
     term_put_tuple_element(socket_term, 1, ref);
     return socket_term;
@@ -1674,7 +1680,7 @@ static term nif_socket_accept(Context *ctx, int argc, term argv[])
         list_remove(&first_item->list_head);
         free(first_item);
 
-        term socket_term = make_accepted_socket_term(new_resource, &ctx->heap, global);
+        term socket_term = make_accepted_socket_term(ctx, new_resource);
         result = term_alloc_tuple(2, &ctx->heap);
         term_put_tuple_element(result, 0, OK_ATOM);
         term_put_tuple_element(result, 1, socket_term);

src/libAtomVM/posix_nifs.c

@@ -314,7 +314,8 @@ static term nif_atomvm_posix_open(Context *ctx, int argc, term argv[])
         if (UNLIKELY(memory_ensure_free_opt(ctx, TUPLE_SIZE(2) + TERM_BOXED_RESOURCE_SIZE, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {
             RAISE_ERROR(OUT_OF_MEMORY_ATOM);
         }
-        term obj = term_from_resource(fd_obj, &ctx->heap);
+        term obj = enif_make_resource(erl_nif_env_from_context(ctx), fd_obj);
+        enif_release_resource(fd_obj);
         result = term_alloc_tuple(2, &ctx->heap);
         term_put_tuple_element(result, 0, OK_ATOM);
         term_put_tuple_element(result, 1, obj);
@@ -675,7 +676,8 @@ static term nif_atomvm_posix_opendir(Context *ctx, int argc, term argv[])
                 != MEMORY_GC_OK)) {
             RAISE_ERROR(OUT_OF_MEMORY_ATOM);
         }
-        term obj = term_from_resource(dir_obj, &ctx->heap);
+        term obj = enif_make_resource(erl_nif_env_from_context(ctx), dir_obj);
+        enif_release_resource(dir_obj);
         result = term_alloc_tuple(2, &ctx->heap);
         term_put_tuple_element(result, 0, OK_ATOM);
         term_put_tuple_element(result, 1, obj);