Skip to content

umm_poison false positive from ISR #8914

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 28, 2023
Merged

umm_poison false positive from ISR #8914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db49c53
umm_poison false positive from ISR
mhightower83 Apr 21, 2023
a9c4ad3
Move get_poisoned() into umm_malloc() and umm_realloc().
mhightower83 Apr 23, 2023
892be86
Merge branch 'master' into pr-fix-umm_poison
mhightower83 Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cores/esp8266/umm_malloc/Notes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,5 +341,16 @@ Enhancement ideas:
* For multiple Heaps builds, add a dedicated function that always reports
DRAM results.


April 22, 2023

The umm_poison logic runs outside the UMM_CRITICAL_* umbrella. When interrupt
routines do alloc calls, it is possible to interrupt an in-progress allocation
just before the poison is set, with a new alloc request resulting in a false
"poison check fail" against the in-progress allocation. The SDK does mallocs
from ISRs. SmartConfig can illustrate this issue.

Move get_poisoned() within UMM_CRITICAL_* in umm_malloc() and umm_realloc().

*/
#endif
7 changes: 5 additions & 2 deletions cores/esp8266/umm_malloc/umm_local.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,11 @@ void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line)

add_poison_size(&size);
ret = umm_realloc(ptr, size);

ret = get_poisoned(ret, size);
/*
"get_poisoned" is now called from umm_realloc while still in a critical
section. Before umm_realloc returned, the pointer offset was adjusted to
the start of the requested buffer.
*/

return ret;
}
Expand Down
26 changes: 19 additions & 7 deletions cores/esp8266/umm_malloc/umm_malloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,8 @@ void *umm_malloc(size_t size) {

ptr = umm_malloc_core(_context, size);

ptr = POISON_CHECK_SET_POISON(ptr, size);

UMM_CRITICAL_EXIT(id_malloc);

return ptr;
Expand Down Expand Up @@ -1068,7 +1070,7 @@ void *umm_realloc(void *ptr, size_t size) {
// Case 2 - block + next block fits EXACTLY
} else if ((blockSize + nextBlockSize) == blocks) {
DBGLOG_DEBUG("exact realloc using next block - %i\n", blocks);
umm_assimilate_up(c);
umm_assimilate_up(_context, c);
STATS__FREE_BLOCKS_UPDATE(-nextBlockSize);
blockSize += nextBlockSize;

Expand All @@ -1087,8 +1089,9 @@ void *umm_realloc(void *ptr, size_t size) {
STATS__FREE_BLOCKS_UPDATE(-prevBlockSize);
STATS__FREE_BLOCKS_ISR_MIN();
blockSize += prevBlockSize;
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); // Fix allocation so ISR poison check is good
UMM_CRITICAL_SUSPEND(id_realloc);
memmove((void *)&UMM_DATA(c), ptr, curSize);
UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize);
ptr = (void *)&UMM_DATA(c);
UMM_CRITICAL_RESUME(id_realloc);
// Case 5 - prev block + block + next block fits
Expand All @@ -1108,8 +1111,9 @@ void *umm_realloc(void *ptr, size_t size) {
#else
blockSize += (prevBlockSize + nextBlockSize);
#endif
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
UMM_CRITICAL_SUSPEND(id_realloc);
memmove((void *)&UMM_DATA(c), ptr, curSize);
UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize);
ptr = (void *)&UMM_DATA(c);
UMM_CRITICAL_RESUME(id_realloc);

Expand All @@ -1119,8 +1123,9 @@ void *umm_realloc(void *ptr, size_t size) {
void *oldptr = ptr;
if ((ptr = umm_malloc_core(_context, size))) {
DBGLOG_DEBUG("realloc %i to a bigger block %i, copy, and free the old\n", blockSize, blocks);
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
UMM_CRITICAL_SUSPEND(id_realloc);
memcpy(ptr, oldptr, curSize);
UMM_POISON_MEMCPY(ptr, oldptr, curSize);
UMM_CRITICAL_RESUME(id_realloc);
umm_free_core(_context, oldptr);
} else {
Expand Down Expand Up @@ -1181,8 +1186,9 @@ void *umm_realloc(void *ptr, size_t size) {
blockSize = blocks;
#endif
}
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
UMM_CRITICAL_SUSPEND(id_realloc);
memmove((void *)&UMM_DATA(c), ptr, curSize);
UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize);
ptr = (void *)&UMM_DATA(c);
UMM_CRITICAL_RESUME(id_realloc);
} else if (blockSize >= blocks) { // 2
Expand All @@ -1198,8 +1204,9 @@ void *umm_realloc(void *ptr, size_t size) {
void *oldptr = ptr;
if ((ptr = umm_malloc_core(_context, size))) {
DBGLOG_DEBUG("realloc %d to a bigger block %d, copy, and free the old\n", blockSize, blocks);
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
UMM_CRITICAL_SUSPEND(id_realloc);
memcpy(ptr, oldptr, curSize);
UMM_POISON_MEMCPY(ptr, oldptr, curSize);
UMM_CRITICAL_RESUME(id_realloc);
umm_free_core(_context, oldptr);
} else {
Expand All @@ -1223,8 +1230,9 @@ void *umm_realloc(void *ptr, size_t size) {
void *oldptr = ptr;
if ((ptr = umm_malloc_core(_context, size))) {
DBGLOG_DEBUG("realloc %d to a bigger block %d, copy, and free the old\n", blockSize, blocks);
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
UMM_CRITICAL_SUSPEND(id_realloc);
memcpy(ptr, oldptr, curSize);
UMM_POISON_MEMCPY(ptr, oldptr, curSize);
UMM_CRITICAL_RESUME(id_realloc);
umm_free_core(_context, oldptr);
} else {
Expand All @@ -1250,6 +1258,8 @@ void *umm_realloc(void *ptr, size_t size) {

STATS__FREE_BLOCKS_MIN();

ptr = POISON_CHECK_SET_POISON(ptr, size);

/* Release the critical section... */
UMM_CRITICAL_EXIT(id_realloc);

Expand All @@ -1258,6 +1268,7 @@ void *umm_realloc(void *ptr, size_t size) {

/* ------------------------------------------------------------------------ */

#if !defined(UMM_POISON_CHECK) && !defined(UMM_POISON_CHECK_LITE)
void *umm_calloc(size_t num, size_t item_size) {
void *ret;

Expand All @@ -1273,6 +1284,7 @@ void *umm_calloc(size_t num, size_t item_size) {

return ret;
}
#endif

/* ------------------------------------------------------------------------ */

Expand Down
8 changes: 8 additions & 0 deletions cores/esp8266/umm_malloc/umm_malloc_cfg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,9 @@ extern bool umm_poison_check(void);
// Local Additions to better report location in code of the caller.
void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line);
void umm_poison_free_fl(void *ptr, const char *file, int line);
#define POISON_CHECK_SET_POISON(p, s) get_poisoned(p, s)
#define UMM_POISON_SKETCH_PTR(p) ((void*)((uintptr_t)p + sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_BEFORE))
#define UMM_POISON_SKETCH_PTRSZ(s) (s - sizeof(UMM_POISONED_BLOCK_LEN_TYPE) - UMM_POISON_SIZE_BEFORE - UMM_POISON_SIZE_AFTER)
#if defined(UMM_POISON_CHECK_LITE)
/*
* We can safely do individual poison checks at free and realloc and stay
Expand All @@ -637,8 +640,13 @@ void umm_poison_free_fl(void *ptr, const char *file, int line);
#else
#define POISON_CHECK() 1
#define POISON_CHECK_NEIGHBORS(c) do {} while (false)
#define POISON_CHECK_SET_POISON(p, s) (p)
#define UMM_POISON_SKETCH_PTR(p) (p)
#define UMM_POISON_SKETCH_PTRSZ(s) (s)
#endif

#define UMM_POISON_MEMMOVE(t, p, s) memmove(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(s))
#define UMM_POISON_MEMCPY(t, p, s) memcpy(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(s))

#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE)
/*
Expand Down
19 changes: 12 additions & 7 deletions cores/esp8266/umm_malloc/umm_poison.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,11 @@ void *umm_poison_malloc(size_t size) {
add_poison_size(&size);

ret = umm_malloc(size);

ret = get_poisoned(ret, size);
/*
"get_poisoned" is now called from umm_malloc while still in a critical
section. Before umm_malloc returned, the pointer offset was adjusted to
the start of the requested buffer.
*/

return ret;
}
Expand All @@ -177,17 +180,16 @@ void *umm_poison_calloc(size_t num, size_t item_size) {
// Use saturated multiply.
// Rely on umm_malloc to supply the fail response as needed.
size_t size = umm_umul_sat(num, item_size);
size_t request_sz = size;

add_poison_size(&size);

ret = umm_malloc(size);

if (NULL != ret) {
memset(ret, 0x00, size);
memset(ret, 0x00, request_sz);
}

ret = get_poisoned(ret, size);

return ret;
}

Expand All @@ -200,8 +202,11 @@ void *umm_poison_realloc(void *ptr, size_t size) {

add_poison_size(&size);
ret = umm_realloc(ptr, size);

ret = get_poisoned(ret, size);
/*
"get_poisoned" is now called from umm_realloc while still in a critical
section. Before umm_realloc returned, the pointer offset was adjusted to
the start of the requested buffer.
*/

return ret;
}
Expand Down