From c9d63c9f4f137bd5cfbc8848d35edd87c2c82f2d Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Wed, 9 Dec 2020 13:16:41 +0100 Subject: [PATCH 1/4] tests/malloc_thread_safety: new test This test checks whether calling malloc in more than one thread is safe. --- tests/malloc_thread_safety/Makefile | 10 ++ tests/malloc_thread_safety/Makefile.ci | 7 ++ tests/malloc_thread_safety/main.c | 118 +++++++++++++++++++++ tests/malloc_thread_safety/tests/01-run.py | 20 ++++ 4 files changed, 155 insertions(+) create mode 100644 tests/malloc_thread_safety/Makefile create mode 100644 tests/malloc_thread_safety/Makefile.ci create mode 100644 tests/malloc_thread_safety/main.c create mode 100755 tests/malloc_thread_safety/tests/01-run.py 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)) From 902aa29b62d0a701861bc196ff190a3f6fd487a7 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 10 Dec 2020 09:39:44 +0100 Subject: [PATCH 2/4] sys/malloc_thread_safe: split out of cpu/atmega_common Split out Gunar Schorcht's clever approach to provide thread safe malloc for AVR into a system module and make AVR depend on this. This allows other platforms to also use this. --- cpu/atmega_common/Makefile.dep | 5 ++ .../avr_libc_extra/posix_unistd.c | 41 -------------- makefiles/arch/atmega.inc.mk | 3 - sys/Kconfig | 1 + sys/Makefile.include | 4 ++ sys/malloc_thread_safe/Kconfig | 18 ++++++ sys/malloc_thread_safe/Makefile | 1 + sys/malloc_thread_safe/Makefile.include | 2 + sys/malloc_thread_safe/doc.txt | 26 +++++++++ sys/malloc_thread_safe/malloc_wrappers.c | 55 +++++++++++++++++++ 10 files changed, 112 insertions(+), 44 deletions(-) create mode 100644 sys/malloc_thread_safe/Kconfig create mode 100644 sys/malloc_thread_safe/Makefile create mode 100644 sys/malloc_thread_safe/Makefile.include create mode 100644 sys/malloc_thread_safe/doc.txt create mode 100644 sys/malloc_thread_safe/malloc_wrappers.c 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/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..be81c82066 --- /dev/null +++ b/sys/malloc_thread_safe/malloc_wrappers.c @@ -0,0 +1,55 @@ +/* + * 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 "irq.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); + +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; +} + +/** @} */ From c790e2eb6da10c141cfb34642ea5e140a47432b1 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 10 Dec 2020 09:43:55 +0100 Subject: [PATCH 3/4] sys/malloc_thread_safety: use mutex Disabling IRQs during malloc() provides mutually exclusive access and even is safe from IRQ context, but is suboptimal for real time scenarios. Instead, the implementation is changed to use a mutex to provide mutually exclusive access. As a result, calls to malloc() and free() from IRQ context no longer is possible. But this this is a really horrible idea to begin with, the impact should be minimal and the improved real time properties of the system should make it a good trade-off. An assert() is added to allow easy detection of regressions and, hence, aid users to fix their code. --- sys/malloc_thread_safe/malloc_wrappers.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/sys/malloc_thread_safe/malloc_wrappers.c b/sys/malloc_thread_safe/malloc_wrappers.c index be81c82066..138dae65a2 100644 --- a/sys/malloc_thread_safe/malloc_wrappers.c +++ b/sys/malloc_thread_safe/malloc_wrappers.c @@ -14,41 +14,49 @@ * @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) { - unsigned state = irq_disable(); + assert(!irq_is_in()); + mutex_lock(&_lock); void *ptr = __real_malloc(size); - irq_restore(state); + mutex_unlock(&_lock); return ptr; } void __wrap_free(void *ptr) { - unsigned state = irq_disable(); + assert(!irq_is_in()); + mutex_lock(&_lock); __real_free(ptr); - irq_restore(state); + mutex_unlock(&_lock); } void *__wrap_calloc(size_t nmemb, size_t size) { - unsigned state = irq_disable(); + assert(!irq_is_in()); + mutex_lock(&_lock); void *ptr = __real_calloc(nmemb, size); - irq_restore(state); + mutex_unlock(&_lock); return ptr; } void *__wrap_realloc(void *ptr, size_t size) { - unsigned state = irq_disable(); + assert(!irq_is_in()); + mutex_lock(&_lock); void *new = __real_realloc(ptr, size); - irq_restore(state); + mutex_unlock(&_lock); return new; } From 09b41d2e1e726437a742cd9ccc43e5269dea2740 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 10 Dec 2020 09:59:21 +0100 Subject: [PATCH 4/4] cpu/cortexm_common: use malloc_thread_safe module --- cpu/cortexm_common/Kconfig | 1 + cpu/cortexm_common/Makefile.dep | 3 +++ 2 files changed, 4 insertions(+) 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