Fix refcount calculation when copying data to heaps

Ref count should be incremented whenever the resource 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;
 }

tests/CMakeLists.txt

@@ -23,17 +23,20 @@ project (tests)
 
 add_executable(test-erlang test.c)
 add_executable(test-enif test-enif.c)
+add_executable(test-heap test-heap.c)
 add_executable(test-mailbox test-mailbox.c)
 add_executable(test-structs test-structs.c)
 
 target_compile_features(test-erlang PUBLIC c_std_11)
 target_compile_features(test-enif PUBLIC c_std_11)
+target_compile_features(test-heap PUBLIC c_std_11)
 target_compile_features(test-mailbox PUBLIC c_std_11)
 target_compile_features(test-structs PUBLIC c_std_11)
 
 if(CMAKE_COMPILER_IS_GNUCC)
     target_compile_options(test-erlang PUBLIC -Wall -pedantic -Wextra -ggdb)
     target_compile_options(test-enif PUBLIC -Wall -pedantic -Wextra -ggdb)
+    target_compile_options(test-heap PUBLIC -Wall -pedantic -Wextra -ggdb)
     target_compile_options(test-mailbox PUBLIC -Wall -pedantic -Wextra -ggdb)
     target_compile_options(test-structs PUBLIC -Wall -pedantic -Wextra -ggdb)
 endif()
@@ -46,6 +49,7 @@ if(${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
         find_library(LIBRT rt REQUIRED)
         target_link_libraries(test-erlang PRIVATE ${LIBRT})
         target_link_libraries(test-enif PRIVATE ${LIBRT})
+        target_link_libraries(test-heap PRIVATE ${LIBRT})
         target_link_libraries(test-mailbox PRIVATE ${LIBRT})
         target_link_libraries(test-structs PRIVATE ${LIBRT})
     else()
@@ -58,6 +62,7 @@ include(MbedTLS)
 if (MbedTLS_FOUND)
     target_link_libraries(test-erlang PRIVATE MbedTLS::mbedtls)
     target_link_libraries(test-enif PRIVATE MbedTLS::mbedtls)
+    target_link_libraries(test-heap PRIVATE MbedTLS::mbedtls)
     target_link_libraries(test-mailbox PRIVATE MbedTLS::mbedtls)
     target_link_libraries(test-structs PRIVATE MbedTLS::mbedtls)
 endif()
@@ -73,6 +78,7 @@ if((${CMAKE_SYSTEM_NAME} STREQUAL "Darwin") OR
    (${CMAKE_SYSTEM_NAME} STREQUAL "DragonFly"))
     target_include_directories(test-erlang PRIVATE ../src/platforms/generic_unix/lib)
     target_include_directories(test-enif PRIVATE ../src/platforms/generic_unix/lib)
+    target_include_directories(test-heap PRIVATE ../src/platforms/generic_unix/lib)
     target_include_directories(test-mailbox PRIVATE ../src/platforms/generic_unix/lib)
     target_include_directories(test-structs PRIVATE ../src/platforms/generic_unix/lib)
 else()
@@ -81,10 +87,12 @@ endif()
 
 target_include_directories(test-erlang PRIVATE ../src/libAtomVM)
 target_include_directories(test-enif PRIVATE ../src/libAtomVM)
+target_include_directories(test-heap PRIVATE ../src/libAtomVM)
 target_include_directories(test-mailbox PRIVATE ../src/libAtomVM)
 target_include_directories(test-structs PRIVATE ../src/libAtomVM)
 target_link_libraries(test-erlang PRIVATE libAtomVM libAtomVM${PLATFORM_LIB_SUFFIX})
 target_link_libraries(test-enif PRIVATE libAtomVM libAtomVM${PLATFORM_LIB_SUFFIX})
+target_link_libraries(test-heap PRIVATE libAtomVM libAtomVM${PLATFORM_LIB_SUFFIX})
 target_link_libraries(test-mailbox PRIVATE libAtomVM libAtomVM${PLATFORM_LIB_SUFFIX})
 target_link_libraries(test-structs PRIVATE libAtomVM libAtomVM${PLATFORM_LIB_SUFFIX})
 
@@ -106,10 +114,12 @@ if (COVERAGE)
     include(CodeCoverage)
     append_coverage_compiler_flags_to_target(test-erlang)
     append_coverage_compiler_flags_to_target(test-enif)
+    append_coverage_compiler_flags_to_target(test-heap)
     append_coverage_compiler_flags_to_target(test-mailbox)
     append_coverage_compiler_flags_to_target(test-structs)
     append_coverage_linker_flags_to_target(test-erlang)
     append_coverage_linker_flags_to_target(test-enif)
+    append_coverage_linker_flags_to_target(test-heap)
     append_coverage_linker_flags_to_target(test-mailbox)
     append_coverage_linker_flags_to_target(test-structs)
     if (CMAKE_COMPILER_IS_GNUCC)