Fix newlib malloc thread safety issue

lib/libc/newlib/libc-hooks.c

@@ -16,6 +16,7 @@
 #include <app_memory/app_memdomain.h>
 #include <init.h>
 #include <sys/sem.h>
+#include <sys/mutex.h>
 #include <sys/mem_manage.h>
 
 #define LIBC_BSS	K_APP_BMEM(z_libc_partition)
@@ -267,14 +268,10 @@ __weak void _exit(int status)
 	}
 }
 
-static LIBC_DATA SYS_SEM_DEFINE(heap_sem, 1, 1);
-
 void *_sbrk(intptr_t count)
 {
 	void *ret, *ptr;
 
-	/* coverity[CHECKED_RETURN] */
-	sys_sem_take(&heap_sem, K_FOREVER);
 	ptr = ((char *)HEAP_BASE) + heap_sz;
 
 	if ((heap_sz + count) < MAX_HEAP_SIZE) {
@@ -284,13 +281,22 @@ void *_sbrk(intptr_t count)
 		ret = (void *)-1;
 	}
 
-	/* coverity[CHECKED_RETURN] */
-	sys_sem_give(&heap_sem);
-
 	return ret;
 }
 __weak FUNC_ALIAS(_sbrk, sbrk, void *);
 
+static LIBC_DATA SYS_MUTEX_DEFINE(heap_mutex);
+
+void __malloc_lock(struct _reent *reent)
+{
+	sys_mutex_lock(&heap_mutex, K_FOREVER);
+}
+
+void __malloc_unlock(struct _reent *reent)
+{
+	sys_mutex_unlock(&heap_mutex);
+}
+
 __weak int *__errno(void)
 {
 	return z_errno();

tests/lib/newlib/thread_safety/CMakeLists.txt

@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.13.1)
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+project(newlib_thread_safety)
+
+FILE(GLOB app_sources src/*.c)
+target_sources(app PRIVATE ${app_sources})

tests/lib/newlib/thread_safety/prj.conf

@@ -0,0 +1,3 @@
+CONFIG_ZTEST=y
+CONFIG_NEWLIB_LIBC=y
+CONFIG_TIMESLICE_SIZE=1

tests/lib/newlib/thread_safety/src/main.c

@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2021 Stephanos Ioannidis <[email protected]>
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/*
+ * @file Newlib thread safety test
+ *
+ * This file contains a set of tests to verify that the C standard functions
+ * provided by newlib are thread safe (i.e. synchronised) and that the thread-
+ * specific contexts are properly handled (i.e. re-entrant).
+ */
+
+#include <zephyr.h>
+#include <ztest.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#define THREAD_COUNT	(64)
+#define STACK_SIZE	(512 + CONFIG_TEST_EXTRA_STACKSIZE)
+#define TEST_INTERVAL	(30) /* seconds */
+
+static struct k_thread tdata[THREAD_COUNT];
+static K_THREAD_STACK_ARRAY_DEFINE(tstack, THREAD_COUNT, STACK_SIZE);
+
+void malloc_thread(void *p1, void *p2, void *p3)
+{
+	static atomic_t count;
+	bool *aborted = p1;
+	int *volatile ptr;
+	int val;
+
+	while (*aborted == false) {
+		/* Compute unique value specific to this iteration. */
+		val = atomic_inc(&count);
+
+		/* Allocate memory block and write a unique value to it. */
+		ptr = malloc(sizeof(int));
+		zassert_not_null(ptr, "Out of memory");
+		*ptr = val;
+
+		/* Busy wait to increase the likelihood of preemption. */
+		k_busy_wait(10);
+
+		/*
+		 * Verify that the unique value previously written to the
+		 * memory block is valid.  This value will become corrupted if
+		 * the newlib heap is not properly synchronised.
+		 */
+		zassert_equal(*ptr, val, "Corrupted memory block");
+
+		/* Free memory block. */
+		free(ptr);
+	}
+}
+
+/**
+ * @brief Test thread safety of newlib memory management functions
+ *
+ * This test calls the malloc() and free() functions from multiple threads to
+ * verify that no corruption occurs in the newlib memory heap.
+ */
+void test_malloc_thread_safety(void)
+{
+	int i;
+	k_tid_t tid[THREAD_COUNT];
+	bool aborted = false;
+
+	/* Create worker threads. */
+	for (i = 0; i < ARRAY_SIZE(tid); i++) {
+		tid[i] = k_thread_create(&tdata[i], tstack[i], STACK_SIZE,
+					 malloc_thread, &aborted, NULL, NULL,
+					 K_PRIO_PREEMPT(0), 0, K_NO_WAIT);
+	}
+
+	TC_PRINT("Created %d worker threads.\n", THREAD_COUNT);
+
+	/* Wait and see if any failures occur. */
+	TC_PRINT("Waiting %d seconds to see if any failures occur ...\n",
+		 TEST_INTERVAL);
+
+	k_sleep(K_SECONDS(TEST_INTERVAL));
+
+	/* Abort all worker threads. */
+	aborted = true;
+
+	for (i = 0; i < ARRAY_SIZE(tid); i++) {
+		k_thread_join(tid[i], K_FOREVER);
+	}
+}
+
+void test_main(void)
+{
+	ztest_test_suite(newlib_thread_safety,
+			 ztest_unit_test(test_malloc_thread_safety));
+
+	ztest_run_test_suite(newlib_thread_safety);
+}

tests/lib/newlib/thread_safety/testcase.yaml

@@ -0,0 +1,13 @@
+common:
+  filter: TOOLCHAIN_HAS_NEWLIB == 1
+  min_ram: 64
+  tags: clib newlib
+
+tests:
+  libraries.libc.newlib.thread_safety:
+    slow: true
+  libraries.libc.newlib_nano.thread_safety:
+    slow: true
+    filter: CONFIG_HAS_NEWLIB_LIBC_NANO
+    extra_configs:
+      - CONFIG_NEWLIB_LIBC_NANO=y