Merge pull request #15606 from maribu/malloc-newlib-picolibc

sys/malloc_thread_safe: new module

cpu/atmega_common/Makefile.dep

@@ -7,6 +7,11 @@ USEMODULE += atmega_common
 # peripheral drivers are linked into the final binary
 USEMODULE += atmega_common_periph
 
+# The AVR-libc provides no thread safe malloc implementation and has no hooks
+# to inject. Use malloc_thread_safe to link calls to malloc to safe wrappers
+# instead.
+USEMODULE += malloc_thread_safe
+
 # the atmel port uses stdio_uart by default
 ifeq (,$(filter stdio_% slipdev_stdio,$(USEMODULE)))
   USEMODULE += stdio_uart

cpu/atmega_common/avr_libc_extra/posix_unistd.c

@@ -177,45 +177,4 @@ ssize_t write(int fd, const void *src, size_t count)
 #endif
 }
 
-/*
- * Following functions are wrappers around the according avr-libc system
- * functions to avoid their preemption by disabling the interrupts for the
- * time of their execution.
- */
-extern void *__real_malloc(size_t size);
-extern void __real_free(void *ptr);
-extern void *__real_calloc(size_t nmemb, size_t size);
-extern void *__real_realloc(void *ptr, size_t size);
-
-void *__wrap_malloc(size_t size)
-{
-    unsigned state = irq_disable();
-    void *ptr = __real_malloc(size);
-    irq_restore(state);
-    return ptr;
-}
-
-void __wrap_free(void *ptr)
-{
-    unsigned state = irq_disable();
-    __real_free(ptr);
-    irq_restore(state);
-}
-
-void *__wrap_calloc(size_t nmemb, size_t size)
-{
-    unsigned state = irq_disable();
-    void *ptr = __real_calloc(nmemb, size);
-    irq_restore(state);
-    return ptr;
-}
-
-void *__wrap_realloc(void *ptr, size_t size)
-{
-    unsigned state = irq_disable();
-    void *new = __real_realloc(ptr, size);
-    irq_restore(state);
-    return new;
-}
-
 /** @} */

cpu/cortexm_common/Kconfig

@@ -9,6 +9,7 @@ config MODULE_CORTEXM_COMMON
     default y if CPU_CORE_CORTEX_M
     depends on TEST_KCONFIG
     select MODULE_PERIPH
+    select MODULE_MALLOC_THREAD_SAFE
     help
         Common code for Cortex-M cores.
 

cpu/cortexm_common/Makefile.dep

@@ -32,3 +32,6 @@ endif
 ifeq (1, $(DEVELHELP))
   FEATURES_OPTIONAL += cortexm_mpu
 endif
+
+# Make calls to malloc and friends thread-safe
+USEMODULE += malloc_thread_safe

makefiles/arch/atmega.inc.mk

@@ -23,9 +23,6 @@ LDSCRIPT_COMPAT = $(if $(shell $(TARGET_ARCH)-ld --verbose | grep __TEXT_REGION_
                     -T$(RIOTCPU)/$(CPU)/ldscripts_compat/avr_2.26.ld)
 LINKFLAGS += $(LDSCRIPT_COMPAT)
 
-# use the wrapper functions for following avr-libc functions
-LINKFLAGS += -Wl,-wrap=malloc -Wl,-wrap=calloc -Wl,-wrap=realloc -Wl,-wrap=free
-
 ifeq ($(LTO),1)
   # avr-gcc <4.8.3 has a bug when using LTO which causes a warning to be printed always:
   # '_vector_25' appears to be a misspelled signal handler [enabled by default]

sys/Kconfig

@@ -17,6 +17,7 @@ rsource "entropy_source/Kconfig"
 rsource "event/Kconfig"
 rsource "fmt/Kconfig"
 rsource "isrpipe/Kconfig"
+rsource "malloc_thread_safe/Kconfig"
 rsource "net/Kconfig"
 rsource "Kconfig.newlib"
 rsource "Kconfig.stdio"

sys/Makefile.include

@@ -21,6 +21,10 @@ ifneq (,$(filter gnrc_pktbuf,$(USEMODULE)))
   include $(RIOTBASE)/sys/net/gnrc/pktbuf/Makefile.include
 endif
 
+ifneq (,$(filter malloc_thread_safe,$(USEMODULE)))
+  include $(RIOTBASE)/sys/malloc_thread_safe/Makefile.include
+endif
+
 ifneq (,$(filter posix_headers,$(USEMODULE)))
   USEMODULE_INCLUDES += $(RIOTBASE)/sys/posix/include
 endif

sys/malloc_thread_safe/Kconfig

@@ -0,0 +1,18 @@
+# Copyright (C) 2020 Otto-von-Guericke-Universität Magdeburg
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+#
+
+config MODULE_MALLOC_THREAD_SAFE
+    bool
+    depends on TEST_KCONFIG
+    help
+        This module provides wrappers for malloc(), calloc(), realloc(), and
+        free() which guarantee mutually exclusive access to heap data
+        structures. This linker is also instructed to redirect all calls to
+        the corresponding wrappers. As a result, all allocations become thread
+        safe without touching the application code or the c library. This module
+        is intended to be pulled in automatically if needed. Hence, applications
+        never should manually use it.

sys/malloc_thread_safe/Makefile

@@ -0,0 +1 @@
+include $(RIOTBASE)/Makefile.base

sys/malloc_thread_safe/Makefile.include

@@ -0,0 +1,2 @@
+# use the wrapper functions for malloc and friends to provide thread-safety
+LINKFLAGS += -Wl,-wrap=malloc -Wl,-wrap=calloc -Wl,-wrap=realloc -Wl,-wrap=free

sys/malloc_thread_safe/doc.txt

@@ -0,0 +1,26 @@
+/**
+@defgroup   sys_malloc_ts           Thread-safe wrappers for malloc and friends
+@ingroup    sys
+@brief      This module provides wrappers for malloc, calloc, realloc and free
+            that provide mutually exclusive access to those functions.
+@warning    This module is automatically selected, if needed. Never add it
+            manually.
+
+# Background
+
+Without support of the OS (or resorting to disabling IRQs), the standard C
+library is unable to guarantee that at most one thread at a time accesses the
+heap management data structures. Some C libraries provide hooks for locking
+(e.g. picolibc and newlib do so optionally), others (e.g. AVR libc) don't.
+By providing wrapper functions for `malloc()` and friends and instructing the
+linker to link to those instead of their actual implementations, we can provide
+thread safe access to the heap regardless of C libraries support.
+
+
+# Usage
+
+This module is intended to be use by platforms not providing the required
+locking with other means automatically. Hence, application developers and users
+should never select this module by hand.
+
+ */

sys/malloc_thread_safe/malloc_wrappers.c

@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2019 Gunar Schorcht
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @{
+ *
+ * @file
+ * @brief   Implements various POSIX syscalls
+ * @author  Gunar Schorcht <gunar@schorcht.net>
+ */
+
+#include "assert.h"
+#include "irq.h"
+#include "mutex.h"
+
+extern void *__real_malloc(size_t size);
+extern void __real_free(void *ptr);
+extern void *__real_calloc(size_t nmemb, size_t size);
+extern void *__real_realloc(void *ptr, size_t size);
+
+static mutex_t _lock;
+
+void *__wrap_malloc(size_t size)
+{
+    assert(!irq_is_in());
+    mutex_lock(&_lock);
+    void *ptr = __real_malloc(size);
+    mutex_unlock(&_lock);
+    return ptr;
+}
+
+void __wrap_free(void *ptr)
+{
+    assert(!irq_is_in());
+    mutex_lock(&_lock);
+    __real_free(ptr);
+    mutex_unlock(&_lock);
+}
+
+void *__wrap_calloc(size_t nmemb, size_t size)
+{
+    assert(!irq_is_in());
+    mutex_lock(&_lock);
+    void *ptr = __real_calloc(nmemb, size);
+    mutex_unlock(&_lock);
+    return ptr;
+}
+
+void *__wrap_realloc(void *ptr, size_t size)
+{
+    assert(!irq_is_in());
+    mutex_lock(&_lock);
+    void *new = __real_realloc(ptr, size);
+    mutex_unlock(&_lock);
+    return new;
+}
+
+/** @} */

tests/malloc_thread_safety/Makefile

@@ -0,0 +1,10 @@
+include ../Makefile.tests_common
+
+USEMODULE += xtimer
+
+include $(RIOTBASE)/Makefile.include
+
+# Only newlib and picolib provide mallinfo
+ifeq (,$(filter newlib picolibc,$(USEMODULE)))
+  CFLAGS += -DNO_MALLINFO
+endif

tests/malloc_thread_safety/Makefile.ci

@@ -0,0 +1,7 @@
+BOARD_INSUFFICIENT_MEMORY := \
+    arduino-duemilanove \
+    arduino-nano \
+    arduino-uno \
+    atmega328p \
+    nucleo-l011k4 \
+    #

tests/malloc_thread_safety/main.c

@@ -0,0 +1,118 @@
+/*
+ * Copyright (C) 2020 Otto-von-Guericke-Universität Magdeburg
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     tests
+ * @{
+ *
+ * @file
+ * @brief       Test application for checking whether malloc is thread-safe
+ *
+ * @author      Marian Buschsieweke <marian.buschsieweke@ovgu.de>
+ * @}
+ */
+
+#include <errno.h>
+#include <stdint.h>
+/* keep stdatomic.h after stdint.h for buggy toolchains */
+#include <stdatomic.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "architecture.h"
+#include "clist.h"
+#include "sched.h"
+#include "test_utils/expect.h"
+#include "thread.h"
+#include "xtimer.h"
+
+#ifndef NO_MALLINFO
+#include <malloc.h>
+#endif
+
+static char WORD_ALIGNED t1_stack[THREAD_STACKSIZE_SMALL];
+static char WORD_ALIGNED t2_stack[THREAD_STACKSIZE_SMALL];
+static atomic_uint_least8_t is_running = ATOMIC_VAR_INIT(1);
+
+void * t1_t2_func(void *arg)
+{
+    (void)arg;
+    while (atomic_load(&is_running)) {
+        int *chunk1 = malloc(sizeof(int) * 1);
+        int *chunk2 = malloc(sizeof(int) * 2);
+        int *chunk3 = malloc(sizeof(int) * 4);
+        int *chunk4 = malloc(sizeof(int) * 8);
+        expect(chunk1 && chunk2 && chunk3 && chunk4);
+        free(chunk1);
+        free(chunk2);
+        free(chunk3);
+        free(chunk4);
+    }
+
+    return NULL;
+}
+
+static void evil_schedule_hack_dont_do_this_at_home(uint8_t prio)
+{
+    extern clist_node_t sched_runqueues[SCHED_PRIO_LEVELS];
+    clist_lpoprpush(&sched_runqueues[prio]);
+}
+
+int main(void)
+{
+    kernel_pid_t t1, t2;
+    puts(
+        "Test Application for multithreaded use of malloc()\n"
+        "==================================================\n"
+        "\n"
+        "This test will run duelling threads allocating and freeing memory.\n"
+        "The running thread is interrupted every millisecond and the other\n"
+        "threads gets scheduled. Eventually, this should yield to memory\n"
+        "corruption unless proper guards are in place preventing them. After\n"
+        "ca. two seconds without crash, the test is considered as passing.\n"
+    );
+
+#ifndef NO_MALLINFO
+    struct mallinfo pre = mallinfo();
+#else
+    puts("WARNING: Use of mallinfo() disabled.\n");
+#endif
+
+    t1 = thread_create(t1_stack, sizeof(t1_stack), THREAD_PRIORITY_MAIN + 1,
+                       THREAD_CREATE_STACKTEST, t1_t2_func, NULL, "t1");
+    t2 = thread_create(t2_stack, sizeof(t2_stack), THREAD_PRIORITY_MAIN + 1,
+                       THREAD_CREATE_STACKTEST, t1_t2_func, NULL, "t2");
+    expect((t1 != KERNEL_PID_UNDEF) && (t2 != KERNEL_PID_UNDEF));
+
+    for (uint16_t i = 0; i < 2 * MS_PER_SEC; i++) {
+        xtimer_usleep(US_PER_MS);
+        /* shuffle t1 and t2 in their run queue. This should eventually hit
+         * during a call to malloc() or free() and disclose any missing
+         * guards */
+        evil_schedule_hack_dont_do_this_at_home(THREAD_PRIORITY_MAIN + 1);
+    }
+
+    /* Don't keep threads spinning */
+    atomic_store(&is_running, 0);
+    /* Give threads time to terminate */
+    xtimer_usleep(10 * US_PER_MS);
+
+#ifndef NO_MALLINFO
+    struct mallinfo post = mallinfo();
+
+    /* RIOT's board or arch support hopefully doesn't use malloc, so there
+     * should be zero bytes allocated prior to the first call to malloc() in
+     * this test. But let's be forgiving and just expect that the number of
+     * allocated bytes before and after the test is equal.
+     */
+    expect(pre.uordblks == post.uordblks);
+#endif
+    puts("TEST PASSED");
+
+    return 0;
+}

tests/malloc_thread_safety/tests/01-run.py

@@ -0,0 +1,20 @@
+#!/usr/bin/env python3
+
+#  Copyright (C) 2020 Otto-von-Guericke-Universität Magdeburg
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+
+# @author      Marian Buschsieweke <marian.buschsieweke@ovgu.de>
+
+import sys
+from testrunner import run
+
+
+def testfunc(child):
+    child.expect("TEST PASSED")
+
+
+if __name__ == "__main__":
+    sys.exit(run(testfunc))