Fix refcount calculation when copying data to heaps

.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);

src/libAtomVM/refc_binary.c

@@ -41,7 +41,7 @@ struct RefcBinary *refc_binary_create_resource(size_t size, struct ResourceType
         return NULL;
     }
     list_init(&refc->head);
-    refc->ref_count = 1;
+    refc->ref_count = 0;
     refc->size = size;
     refc->resource_type = resource_type;