From 48cf1c9c80783d5c9d25e13309e9f34b3e81abf1 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Wed, 8 Sep 2021 10:11:17 +0100 Subject: [PATCH 1/2] rtos: Thread: Make stack allocation failure runtime catchable When a Thread object's stack memory is not provided, its `start()` member function dynamically allocates its stack from the heap. If allocation fails, there is no way to catch it because * `std::nothrow` is missing after the `new` keyword. As Mbed OS is built with `-fno-exceptions` (C++ exceptions disabled), failed allocation results in an unrecoverable fault. * The attempted `nullptr` check, which doesn't work anyway due to the first point, is an assertion instead of error returning. Assertions should be used as a development tool to ensure code behaves correctly. But out-of-memory is a completely valid runtime situation. This commit adds the missing `std::nothrow`, and makes `Thread::start()` return `osErrorNoMemory` if allocation fails so the caller can handle it. Note: A case when a thread should never fail due to lack of memory is the main thread. But the main thread's stack is a pre-allocated array in the static memory, passed to the `Thread()` constructor during thread creation, so it's not impacted by this change. --- rtos/include/rtos/Thread.h | 3 ++- rtos/source/Thread.cpp | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/rtos/include/rtos/Thread.h b/rtos/include/rtos/Thread.h index 1aa72de714..2083e32e02 100644 --- a/rtos/include/rtos/Thread.h +++ b/rtos/include/rtos/Thread.h @@ -131,7 +131,8 @@ public: /** Starts a thread executing the specified function. @param task function to be executed by this thread. - @return status code that indicates the execution status of the function. + @return status code that indicates the execution status of the function, + or osErrorNoMemory if stack allocation failed. @note a thread can only be started once @note You cannot call this function ISR context. diff --git a/rtos/source/Thread.cpp b/rtos/source/Thread.cpp index 8d1fa927a4..3b9a8d9aaf 100644 --- a/rtos/source/Thread.cpp +++ b/rtos/source/Thread.cpp @@ -81,8 +81,11 @@ osStatus Thread::start(mbed::Callback task) } if (_attr.stack_mem == nullptr) { - _attr.stack_mem = new uint32_t[_attr.stack_size / sizeof(uint32_t)]; - MBED_ASSERT(_attr.stack_mem != nullptr); + _attr.stack_mem = new (std::nothrow) uint32_t[_attr.stack_size / sizeof(uint32_t)]; + if (_attr.stack_mem == nullptr) { + _mutex.unlock(); + return osErrorNoMemory; + } } //Fill the stack with a magic word for maximum usage checking From 0b868d5a1a69bdbb958db6431caa02dcd5107851 Mon Sep 17 00:00:00 2001 From: Lingkai Dong Date: Tue, 7 Sep 2021 15:59:03 +0100 Subject: [PATCH 2/2] General block device test: Fix thread stack allocation The test case for multithreaded erase/program/read allocates a few Thread objects from the heap and starts them. It has a few problems: * To check that there will be enough heap to start a new thread, the test case tries to allocate a dummy buffer of the thread's heap size and then frees it, before starting the thread. Then the thread will allocate its own stack. Such check is not reliable, because threads that are already running also perform additional allocation (when running `test_thread_job()`) and may take away the memory we just checked. * When deleting all threads in a loop, the loop boundary misses the last thread if the last thread object was allocated but not started (i.e. due to failed thread stack allocation check). To fix the issues * Start a thread without any allocation test. Following the preceding commit "rtos: Thread: Make stack allocation failure runtime catchable", `Thread::start()` now returns `osErrorNoMemory` if stack allocation fails which we can handle. * Store pointers to all threads in a zero-initialized array, and free all elements at the end of the test. --- .../blockdevice/general_block_device/main.cpp | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/storage/blockdevice/tests/TESTS/blockdevice/general_block_device/main.cpp b/storage/blockdevice/tests/TESTS/blockdevice/general_block_device/main.cpp index ede4a70959..11b3e11d44 100644 --- a/storage/blockdevice/tests/TESTS/blockdevice/general_block_device/main.cpp +++ b/storage/blockdevice/tests/TESTS/blockdevice/general_block_device/main.cpp @@ -421,40 +421,35 @@ void test_multi_threads() osStatus threadStatus; int i_ind, j_ind; - char *dummy; - rtos::Thread **bd_thread = new (std::nothrow) rtos::Thread*[TEST_NUM_OF_THREADS]; - TEST_SKIP_UNLESS_MESSAGE((*bd_thread) != NULL, "not enough heap to run test."); - memset(bd_thread, 0, TEST_NUM_OF_THREADS * sizeof(rtos::Thread *)); + rtos::Thread *bd_thread[TEST_NUM_OF_THREADS] {}; for (i_ind = 0; i_ind < TEST_NUM_OF_THREADS; i_ind++) { bd_thread[i_ind] = new (std::nothrow) rtos::Thread((osPriority_t)((int)osPriorityNormal), TEST_THREAD_STACK_SIZE); - dummy = new (std::nothrow) char[TEST_THREAD_STACK_SIZE]; - if (!bd_thread[i_ind] || !dummy) { - utest_printf("Not enough heap to run Thread %d !\n", i_ind + 1); + if (!bd_thread[i_ind]) { + utest_printf("Not enough heap to create Thread %d\n", i_ind + 1); break; } - delete[] dummy; threadStatus = bd_thread[i_ind]->start(callback(test_thread_job)); - if (threadStatus != 0) { - utest_printf("Thread %d Start Failed!\n", i_ind + 1); + if (threadStatus == osErrorNoMemory) { + utest_printf("Not enough heap to start Thread %d\n", i_ind + 1); + } else if (threadStatus != osOK) { + utest_printf("Thread %d failed to start: %d\n", i_ind + 1, threadStatus); break; } } + // Join threads that successfully started for (j_ind = 0; j_ind < i_ind; j_ind++) { bd_thread[j_ind]->join(); } - if (bd_thread) { - for (j_ind = 0; j_ind < i_ind; j_ind++) { - delete bd_thread[j_ind]; - } - - delete[] bd_thread; + // Delete all threads, even those that failed to start + for (j_ind = 0; j_ind < TEST_NUM_OF_THREADS; j_ind++) { + delete bd_thread[j_ind]; } } #endif