Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix newlib malloc thread safety issue #35227

Merged
merged 3 commits into from May 13, 2021

Conversation

stephanosio
Copy link
Member

@stephanosio stephanosio commented May 12, 2021

This series includes a set of patches to fix the thread safety-related issue when calling the malloc and free functions from multiple threads, as well as a new test to validate the thread safety issue.

Before

*** Booting Zephyr OS build v2.6.0-rc1-35-g44fb1c72e629  ***
Running test suite newlib_thread_safety
===================================================================
START - test_malloc_thread_safety
Created 64 worker threads.
Waiting 30 seconds to see if any failures occur ...

    Assertion failed at ../src/main.c:52: malloc_thread: (*ptr not equal to val)
Corrupted memory block
 FAIL - test_malloc_thread_safety in 18.560 seconds
===================================================================
Test suite newlib_thread_safety failed.
===================================================================
PROJECT EXECUTION FAILED

    Assertion failed at ../src/main.c:52: malloc_thread: (*ptr not equal to val)
Corrupted memory block

    Assertion failed at ../src/main.c:41: malloc_thread: (ptr is NULL)
Out of memory

    Assertion failed at ../src/main.c:41: malloc_thread: (ptr is NULL)
Out of memory

After

*** Booting Zephyr OS build v2.6.0-rc1-73-g55584a9a49ee  ***
Running test suite newlib_thread_safety
===================================================================
START - test_malloc_thread_safety
Created 64 worker threads.
Waiting 30 seconds to see if any failures occur ...
 PASS - test_malloc_thread_safety in 30.15 seconds
===================================================================
Test suite newlib_thread_safety succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL

A more comprehensive series addressing the thread safety and re-entrancy issues for all newlib functions will be submitted in the future -- this series is meant to address only the most critical aspect (i.e. memory management) of the underlying problems in a timely manner.

Related to #21519 and #21518

stephanosio added 2 commits May 12, 2021
This commit adds a lock implementation for the newlib heap memory
management functions (`malloc` and `free`).

The `__malloc_lock` and `__malloc_unlock` functions are called by the
newlib `malloc` and `free` functions to synchronise access to the heap
region.

Without this lock, making use of the `malloc` and `free` functions from
multiple threads will result in the corruption of the heap region.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
This commit removes the lock inside the newlib internal `_sbrk`
function, which is called by `malloc` when additional heap memory is
needed.

This lock is no longer required because any calls to the `malloc`
function are synchronised by the `__malloc_lock` and `__malloc_unlock`
functions.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@github-actions github-actions bot added area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test labels May 12, 2021
@stephanosio stephanosio linked an issue May 12, 2021 that may be closed by this pull request
@stephanosio stephanosio force-pushed the newlib_ts_test branch 2 times, most recently from ae04595 to 973f918 Compare May 12, 2021
@stephanosio
Copy link
Member Author

@stephanosio stephanosio commented May 12, 2021

Please ignore checkpatch warning. It's wrong.

This commit adds a new test to verify the thread safety of the C
standard functions provided by newlib.

Only the memory management functions (malloc and free) are tested at
this time; this test will be further extended in the future, to verify
the thread safety and re-entrancy of all supported newlib functions.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio stephanosio marked this pull request as ready for review May 12, 2021
@stephanosio stephanosio requested a review from nashif as a code owner May 12, 2021
@nashif nashif requested a review from andyross May 12, 2021
@stephanosio stephanosio requested review from carlescufi and ioannisg May 12, 2021
@stephanosio stephanosio added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug labels May 12, 2021
Copy link
Contributor

@andyross andyross left a comment

Looks very reasonable

@@ -291,6 +292,18 @@ void *_sbrk(intptr_t count)
}
__weak FUNC_ALIAS(_sbrk, sbrk, void *);

static LIBC_DATA SYS_MUTEX_DEFINE(heap_mutex);
Copy link
Contributor

@andyross andyross May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When CONFIG_USERSPACE=n, making this a spinlock would be much smaller/faster. But they I guess someday we'll get around to implementing a futex backend for sys_mutex which would have the same property.

galak
galak approved these changes May 13, 2021
@galak galak merged commit 6bc42ae into zephyrproject-rtos:master May 13, 2021
15 of 16 checks passed
@stephanosio stephanosio deleted the newlib_ts_test branch May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants