Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c517bfd

Browse filesBrowse files
mhightower83mhightower83
authored
umm_poison false positive from ISR (#8914)
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, see #3494 (comment) Other Sketches built with Debug: "Serial," that use the SDK's Promiscuous callbacks are also likely to encounter problems. The SDK support for "Promiscuous Receive" allocates memory from an ISR context, occasionally interrupting the poison wrapper code before it finishes setting the poison fences resulting in a false poison failed event.
1 parent ad9aaef commit c517bfd
Copy full SHA for c517bfd

File tree

Expand file treeCollapse file tree

5 files changed

+55
-16
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+55
-16
lines changed

‎cores/esp8266/umm_malloc/Notes.h

Copy file name to clipboardExpand all lines: cores/esp8266/umm_malloc/Notes.h
+11Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,5 +341,16 @@ Enhancement ideas:
341341
* For multiple Heaps builds, add a dedicated function that always reports
342342
DRAM results.
343343

344+
345+
April 22, 2023
346+
347+
The umm_poison logic runs outside the UMM_CRITICAL_* umbrella. When interrupt
348+
routines do alloc calls, it is possible to interrupt an in-progress allocation
349+
just before the poison is set, with a new alloc request resulting in a false
350+
"poison check fail" against the in-progress allocation. The SDK does mallocs
351+
from ISRs. SmartConfig can illustrate this issue.
352+
353+
Move get_poisoned() within UMM_CRITICAL_* in umm_malloc() and umm_realloc().
354+
344355
*/
345356
#endif

‎cores/esp8266/umm_malloc/umm_local.c

Copy file name to clipboardExpand all lines: cores/esp8266/umm_malloc/umm_local.c
+5-2Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,11 @@ void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line)
142142

143143
add_poison_size(&size);
144144
ret = umm_realloc(ptr, size);
145-
146-
ret = get_poisoned(ret, size);
145+
/*
146+
"get_poisoned" is now called from umm_realloc while still in a critical
147+
section. Before umm_realloc returned, the pointer offset was adjusted to
148+
the start of the requested buffer.
149+
*/
147150

148151
return ret;
149152
}

‎cores/esp8266/umm_malloc/umm_malloc.cpp

Copy file name to clipboardExpand all lines: cores/esp8266/umm_malloc/umm_malloc.cpp
+19-7Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -909,6 +909,8 @@ void *umm_malloc(size_t size) {
909909

910910
ptr = umm_malloc_core(_context, size);
911911

912+
ptr = POISON_CHECK_SET_POISON(ptr, size);
913+
912914
UMM_CRITICAL_EXIT(id_malloc);
913915

914916
return ptr;
@@ -1068,7 +1070,7 @@ void *umm_realloc(void *ptr, size_t size) {
10681070
// Case 2 - block + next block fits EXACTLY
10691071
} else if ((blockSize + nextBlockSize) == blocks) {
10701072
DBGLOG_DEBUG("exact realloc using next block - %i\n", blocks);
1071-
umm_assimilate_up(c);
1073+
umm_assimilate_up(_context, c);
10721074
STATS__FREE_BLOCKS_UPDATE(-nextBlockSize);
10731075
blockSize += nextBlockSize;
10741076

@@ -1087,8 +1089,9 @@ void *umm_realloc(void *ptr, size_t size) {
10871089
STATS__FREE_BLOCKS_UPDATE(-prevBlockSize);
10881090
STATS__FREE_BLOCKS_ISR_MIN();
10891091
blockSize += prevBlockSize;
1092+
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size); // Fix allocation so ISR poison check is good
10901093
UMM_CRITICAL_SUSPEND(id_realloc);
1091-
memmove((void *)&UMM_DATA(c), ptr, curSize);
1094+
UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize);
10921095
ptr = (void *)&UMM_DATA(c);
10931096
UMM_CRITICAL_RESUME(id_realloc);
10941097
// Case 5 - prev block + block + next block fits
@@ -1108,8 +1111,9 @@ void *umm_realloc(void *ptr, size_t size) {
11081111
#else
11091112
blockSize += (prevBlockSize + nextBlockSize);
11101113
#endif
1114+
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
11111115
UMM_CRITICAL_SUSPEND(id_realloc);
1112-
memmove((void *)&UMM_DATA(c), ptr, curSize);
1116+
UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize);
11131117
ptr = (void *)&UMM_DATA(c);
11141118
UMM_CRITICAL_RESUME(id_realloc);
11151119

@@ -1119,8 +1123,9 @@ void *umm_realloc(void *ptr, size_t size) {
11191123
void *oldptr = ptr;
11201124
if ((ptr = umm_malloc_core(_context, size))) {
11211125
DBGLOG_DEBUG("realloc %i to a bigger block %i, copy, and free the old\n", blockSize, blocks);
1126+
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
11221127
UMM_CRITICAL_SUSPEND(id_realloc);
1123-
memcpy(ptr, oldptr, curSize);
1128+
UMM_POISON_MEMCPY(ptr, oldptr, curSize);
11241129
UMM_CRITICAL_RESUME(id_realloc);
11251130
umm_free_core(_context, oldptr);
11261131
} else {
@@ -1181,8 +1186,9 @@ void *umm_realloc(void *ptr, size_t size) {
11811186
blockSize = blocks;
11821187
#endif
11831188
}
1189+
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
11841190
UMM_CRITICAL_SUSPEND(id_realloc);
1185-
memmove((void *)&UMM_DATA(c), ptr, curSize);
1191+
UMM_POISON_MEMMOVE((void *)&UMM_DATA(c), ptr, curSize);
11861192
ptr = (void *)&UMM_DATA(c);
11871193
UMM_CRITICAL_RESUME(id_realloc);
11881194
} else if (blockSize >= blocks) { // 2
@@ -1198,8 +1204,9 @@ void *umm_realloc(void *ptr, size_t size) {
11981204
void *oldptr = ptr;
11991205
if ((ptr = umm_malloc_core(_context, size))) {
12001206
DBGLOG_DEBUG("realloc %d to a bigger block %d, copy, and free the old\n", blockSize, blocks);
1207+
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
12011208
UMM_CRITICAL_SUSPEND(id_realloc);
1202-
memcpy(ptr, oldptr, curSize);
1209+
UMM_POISON_MEMCPY(ptr, oldptr, curSize);
12031210
UMM_CRITICAL_RESUME(id_realloc);
12041211
umm_free_core(_context, oldptr);
12051212
} else {
@@ -1223,8 +1230,9 @@ void *umm_realloc(void *ptr, size_t size) {
12231230
void *oldptr = ptr;
12241231
if ((ptr = umm_malloc_core(_context, size))) {
12251232
DBGLOG_DEBUG("realloc %d to a bigger block %d, copy, and free the old\n", blockSize, blocks);
1233+
POISON_CHECK_SET_POISON((void *)&UMM_DATA(c), size);
12261234
UMM_CRITICAL_SUSPEND(id_realloc);
1227-
memcpy(ptr, oldptr, curSize);
1235+
UMM_POISON_MEMCPY(ptr, oldptr, curSize);
12281236
UMM_CRITICAL_RESUME(id_realloc);
12291237
umm_free_core(_context, oldptr);
12301238
} else {
@@ -1250,6 +1258,8 @@ void *umm_realloc(void *ptr, size_t size) {
12501258

12511259
STATS__FREE_BLOCKS_MIN();
12521260

1261+
ptr = POISON_CHECK_SET_POISON(ptr, size);
1262+
12531263
/* Release the critical section... */
12541264
UMM_CRITICAL_EXIT(id_realloc);
12551265

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

12591269
/* ------------------------------------------------------------------------ */
12601270

1271+
#if !defined(UMM_POISON_CHECK) && !defined(UMM_POISON_CHECK_LITE)
12611272
void *umm_calloc(size_t num, size_t item_size) {
12621273
void *ret;
12631274

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

12741285
return ret;
12751286
}
1287+
#endif
12761288

12771289
/* ------------------------------------------------------------------------ */
12781290

‎cores/esp8266/umm_malloc/umm_malloc_cfg.h

Copy file name to clipboardExpand all lines: cores/esp8266/umm_malloc/umm_malloc_cfg.h
+8Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,9 @@ extern bool umm_poison_check(void);
618618
// Local Additions to better report location in code of the caller.
619619
void *umm_poison_realloc_fl(void *ptr, size_t size, const char *file, int line);
620620
void umm_poison_free_fl(void *ptr, const char *file, int line);
621+
#define POISON_CHECK_SET_POISON(p, s) get_poisoned(p, s)
622+
#define UMM_POISON_SKETCH_PTR(p) ((void*)((uintptr_t)p + sizeof(UMM_POISONED_BLOCK_LEN_TYPE) + UMM_POISON_SIZE_BEFORE))
623+
#define UMM_POISON_SKETCH_PTRSZ(s) (s - sizeof(UMM_POISONED_BLOCK_LEN_TYPE) - UMM_POISON_SIZE_BEFORE - UMM_POISON_SIZE_AFTER)
621624
#if defined(UMM_POISON_CHECK_LITE)
622625
/*
623626
* We can safely do individual poison checks at free and realloc and stay
@@ -637,8 +640,13 @@ void umm_poison_free_fl(void *ptr, const char *file, int line);
637640
#else
638641
#define POISON_CHECK() 1
639642
#define POISON_CHECK_NEIGHBORS(c) do {} while (false)
643+
#define POISON_CHECK_SET_POISON(p, s) (p)
644+
#define UMM_POISON_SKETCH_PTR(p) (p)
645+
#define UMM_POISON_SKETCH_PTRSZ(s) (s)
640646
#endif
641647

648+
#define UMM_POISON_MEMMOVE(t, p, s) memmove(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(s))
649+
#define UMM_POISON_MEMCPY(t, p, s) memcpy(UMM_POISON_SKETCH_PTR(t), UMM_POISON_SKETCH_PTR(p), UMM_POISON_SKETCH_PTRSZ(s))
642650

643651
#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE)
644652
/*

‎cores/esp8266/umm_malloc/umm_poison.c

Copy file name to clipboardExpand all lines: cores/esp8266/umm_malloc/umm_poison.c
+12-7Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,11 @@ void *umm_poison_malloc(size_t size) {
163163
add_poison_size(&size);
164164

165165
ret = umm_malloc(size);
166-
167-
ret = get_poisoned(ret, size);
166+
/*
167+
"get_poisoned" is now called from umm_malloc while still in a critical
168+
section. Before umm_malloc returned, the pointer offset was adjusted to
169+
the start of the requested buffer.
170+
*/
168171

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

181185
add_poison_size(&size);
182186

183187
ret = umm_malloc(size);
184188

185189
if (NULL != ret) {
186-
memset(ret, 0x00, size);
190+
memset(ret, 0x00, request_sz);
187191
}
188192

189-
ret = get_poisoned(ret, size);
190-
191193
return ret;
192194
}
193195

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

201203
add_poison_size(&size);
202204
ret = umm_realloc(ptr, size);
203-
204-
ret = get_poisoned(ret, size);
205+
/*
206+
"get_poisoned" is now called from umm_realloc while still in a critical
207+
section. Before umm_realloc returned, the pointer offset was adjusted to
208+
the start of the requested buffer.
209+
*/
205210

206211
return ret;
207212
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.