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.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>

.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

@@ -102,6 +102,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

@@ -166,12 +166,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

@@ -33,6 +33,7 @@ certain VM instructions are used.
 - Fixed compilation with latest debian gcc-arm-none-eabi
 - Fix `network:stop/0` on ESP32 so the network can be started again
 - Fix a memory corruption caused by `binary:split/2,3`
+- Fixed potential crashes or memory leaks caused by a mistake in calculation of reference counts
 
 ## [0.6.5] - 2024-10-15
 

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/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;
 

src/libAtomVM/resources.c

@@ -70,6 +70,9 @@ void *enif_alloc_resource(ErlNifResourceType *type, unsigned size)
     // We add it now to the list of refc binaries, so resource is destroyed at
     // the latest when globalcontext is destroyed
     synclist_append(&type->global->refc_binaries, &refc->head);
+    // OTP semantics suppose the ref count is 1 as each alloc_resource should
+    // be balanced by a release_resource.
+    refc->ref_count = 1;
     return (void *) refc_binary_get_data(refc);
 }
 
@@ -111,8 +114,6 @@ ERL_NIF_TERM enif_make_resource(ErlNifEnv *env, void *obj)
     if (UNLIKELY(memory_erl_nif_env_ensure_free(env, TERM_BOXED_RESOURCE_SIZE) != MEMORY_GC_OK)) {
         AVM_ABORT();
     }
-    struct RefcBinary *refc = refc_binary_from_data(obj);
-    refc_binary_increment_refcount(refc);
     return term_from_resource(obj, &env->heap);
 }
 
@@ -209,8 +210,6 @@ term select_event_make_notification(void *rsrc_obj, uint64_t ref_ticks, bool is_
     term notification = term_alloc_tuple(4, heap);
     term_put_tuple_element(notification, 0, SELECT_ATOM);
     term_put_tuple_element(notification, 1, term_from_resource(rsrc_obj, heap));
-    struct RefcBinary *rsrc_refc = refc_binary_from_data(rsrc_obj);
-    refc_binary_increment_refcount(rsrc_refc);
     term ref;
     if (ref_ticks == 0) {
         ref = UNDEFINED_ATOM;

src/libAtomVM/term.c

@@ -700,6 +700,7 @@ term term_alloc_refc_binary(size_t size, bool is_const, Heap *heap, GlobalContex
             AVM_ABORT();
         }
         boxed_value[3] = (term) refc;
+        refc->ref_count = 1; // added to mso list, increment ref count
         heap->root->mso_list = term_list_init_prepend(boxed_value + 4, ret, heap->root->mso_list);
         synclist_append(&glb->refc_binaries, &refc->head);
     }

src/libAtomVM/term.h

@@ -979,12 +979,12 @@ static inline unsigned long term_binary_size(term t)
  * @details Returns address
  * @return offset (in words).
  */
-static inline void *term_refc_binary_ptr(term refc_binary)
+static inline struct RefcBinary *term_refc_binary_ptr(term refc_binary)
 {
     TERM_DEBUG_ASSERT(term_is_refc_binary(refc_binary));
 
     term *boxed_value = term_to_term_ptr(refc_binary);
-    return (void *) boxed_value[3];
+    return (struct RefcBinary *) boxed_value[3];
 }
 
 /**
@@ -1763,9 +1763,8 @@ static inline term term_get_sub_binary_ref(term t)
  * @details This function creates a resource (obtained from `enif_alloc_resource`)
  * on the heap which must have `TERM_BOXED_RESOURCE_SIZE` free terms.
  *
- * Unlike `enif_make_resource`, this function doesn't increment the reference
- * counter but instead makes the heap own the resource. It will be garbage
- * collected when the heap is destroyed.
+ * This function does increment the reference counter as the resource is
+ * added to the heap's mso list.
  *
  * @param resource resource obtained from `enif_alloc_resource`
  * @param heap the heap to allocate the resource in
@@ -1783,6 +1782,7 @@ static inline term term_from_resource(void *resource, Heap *heap)
     term ret = ((term) boxed_value) | TERM_BOXED_VALUE_TAG;
     boxed_value[3] = (term) refc;
     // Add the resource to the mso list
+    refc->ref_count++;
     heap->root->mso_list = term_list_init_prepend(boxed_value + 4, ret, heap->root->mso_list);
     return ret;
 }

src/platforms/emscripten/src/lib/platform_nifs.c

@@ -229,8 +229,6 @@ static bool get_register_callback_parameters(Context *ctx, int argc, term argv[]
     struct EmscriptenPlatformData *platform = ctx->global->platform_data;
     struct HTMLEventUserDataResource *htmlevent_user_data_resource = enif_alloc_resource(platform->htmlevent_user_data_resource_type, resource_size);
     htmlevent_user_data_resource->target_pid = ctx->process_id;
-    // We don't need to keep resource now because caller will make a term using
-    // enif_make_resource which increments ref count
     // Monitor process so we will unregister & decrement ref count if target dies.
     if (UNLIKELY(enif_monitor_process(erl_nif_env_from_context(ctx), htmlevent_user_data_resource, &ctx->process_id, NULL) != 0)) {
         // If we fail, caller will not make a term, so decrement resource count now to dispose it.
@@ -680,6 +678,7 @@ static EM_BOOL html5api_touch_callback(int eventType, const EmscriptenTouchEvent
             return term_from_emscripten_result(result, ctx);                                                                                                                             \
         }                                                                                                                                                                                \
         term resource_term = enif_make_resource(erl_nif_env_from_context(ctx), resource);                                                                                                \
+        enif_release_resource(resource);                                                                                                                                                 \
         if (UNLIKELY(memory_ensure_free_with_roots(ctx, TUPLE_SIZE(3), 1, &resource_term, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {                                                         \
             RAISE_ERROR(OUT_OF_MEMORY_ATOM);                                                                                                                                             \
         }                                                                                                                                                                                \

src/platforms/emscripten/src/lib/sys.c

@@ -226,7 +226,6 @@ em_promise_t sys_enqueue_emscripten_call_message(GlobalContext *glb, const char
     }
     struct EmscriptenPlatformData *platform = glb->platform_data;
     struct PromiseResource *promise_rsrc = enif_alloc_resource(platform->promise_resource_type, sizeof(struct PromiseResource));
-    enif_keep_resource(promise_rsrc);
     promise_rsrc->promise = promise;
     promise_rsrc->resolved = false;
     message->promise_rsrc = promise_rsrc;
@@ -434,6 +433,7 @@ static void sys_emscripten_send_message(GlobalContext *glb, int target_pid, cons
     term_put_tuple_element(payload, 0, globalcontext_make_atom(glb, is_call ? ATOM_STR("\x4", "call") : ATOM_STR("\x4", "cast")));
     if (is_call) {
         term promise_term = term_from_resource(promise, &heap);
+        enif_release_resource(promise);
         term_put_tuple_element(payload, 1, promise_term);
     }
     term bin = term_from_literal_binary(message, message_len, &heap, glb);