diff --git a/cpu/atmega_common/Makefile.dep b/cpu/atmega_common/Makefile.dep index 598572cee5..64fc150a79 100644 --- a/cpu/atmega_common/Makefile.dep +++ b/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 diff --git a/cpu/atmega_common/avr_libc_extra/posix_unistd.c b/cpu/atmega_common/avr_libc_extra/posix_unistd.c index edce417fbd..05aa350997 100644 --- a/cpu/atmega_common/avr_libc_extra/posix_unistd.c +++ b/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; -} - /** @} */ diff --git a/cpu/cortexm_common/Kconfig b/cpu/cortexm_common/Kconfig index afd3726171..8e30fe3174 100644 --- a/cpu/cortexm_common/Kconfig +++ b/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. diff --git a/cpu/cortexm_common/Makefile.dep b/cpu/cortexm_common/Makefile.dep index 1718d328f8..c07e776efa 100644 --- a/cpu/cortexm_common/Makefile.dep +++ b/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 diff --git a/makefiles/arch/atmega.inc.mk b/makefiles/arch/atmega.inc.mk index a85403f58d..34f6109436 100644 --- a/makefiles/arch/atmega.inc.mk +++ b/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] diff --git a/sys/Kconfig b/sys/Kconfig index b0d9a6037c..24a1eed3f3 100644 --- a/sys/Kconfig +++ b/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" diff --git a/sys/Makefile.include b/sys/Makefile.include index 3067d51c00..38bf6afeac 100644 --- a/sys/Makefile.include +++ b/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 diff --git a/sys/malloc_thread_safe/Kconfig b/sys/malloc_thread_safe/Kconfig new file mode 100644 index 0000000000..d50748ca75 --- /dev/null +++ b/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. diff --git a/sys/malloc_thread_safe/Makefile b/sys/malloc_thread_safe/Makefile new file mode 100644 index 0000000000..48422e909a --- /dev/null +++ b/sys/malloc_thread_safe/Makefile @@ -0,0 +1 @@ +include $(RIOTBASE)/Makefile.base diff --git a/sys/malloc_thread_safe/Makefile.include b/sys/malloc_thread_safe/Makefile.include new file mode 100644 index 0000000000..34e2b2a41e --- /dev/null +++ b/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 diff --git a/sys/malloc_thread_safe/doc.txt b/sys/malloc_thread_safe/doc.txt new file mode 100644 index 0000000000..b5c98642b6 --- /dev/null +++ b/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. + + */ diff --git a/sys/malloc_thread_safe/malloc_wrappers.c b/sys/malloc_thread_safe/malloc_wrappers.c new file mode 100644 index 0000000000..138dae65a2 --- /dev/null +++ b/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 + */ + +#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; +} + +/** @} */ diff --git a/tests/malloc_thread_safety/Makefile b/tests/malloc_thread_safety/Makefile new file mode 100644 index 0000000000..21e686cefe --- /dev/null +++ b/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 diff --git a/tests/malloc_thread_safety/Makefile.ci b/tests/malloc_thread_safety/Makefile.ci new file mode 100644 index 0000000000..ff454e3604 --- /dev/null +++ b/tests/malloc_thread_safety/Makefile.ci @@ -0,0 +1,7 @@ +BOARD_INSUFFICIENT_MEMORY := \ + arduino-duemilanove \ + arduino-nano \ + arduino-uno \ + atmega328p \ + nucleo-l011k4 \ + # diff --git a/tests/malloc_thread_safety/main.c b/tests/malloc_thread_safety/main.c new file mode 100644 index 0000000000..8c88e5086a --- /dev/null +++ b/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 + * @} + */ + +#include +#include +/* keep stdatomic.h after stdint.h for buggy toolchains */ +#include +#include +#include + +#include "architecture.h" +#include "clist.h" +#include "sched.h" +#include "test_utils/expect.h" +#include "thread.h" +#include "xtimer.h" + +#ifndef NO_MALLINFO +#include +#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; +} diff --git a/tests/malloc_thread_safety/tests/01-run.py b/tests/malloc_thread_safety/tests/01-run.py new file mode 100755 index 0000000000..60dd8f94f3 --- /dev/null +++ b/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 + +import sys +from testrunner import run + + +def testfunc(child): + child.expect("TEST PASSED") + + +if __name__ == "__main__": + sys.exit(run(testfunc))