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.

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

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented Apr 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.

This should resolve #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.

Edited: expanded description

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.
@mcspr
Copy link
Collaborator

mcspr commented Apr 22, 2023

Any reason not to move currently separate umm_malloc and get_poisoned into a single operation that happens inside critical enter & exit blocks?
Adding a different false positive, other way around by ignoring the check with some specific counter value, feels kind of weird.

@mhightower83
Copy link
Contributor Author

The current upstream umm_malloc has umm_poison as an outer wrapper. Without searching through all the old stuff, it used to be more mixed together. I have already started to blur that separation with POISON_CHECK_NEIGHBORS() in umm_malloc(). I'll look at moving get_poisoned() in as well. I will need to think about this a bit. realloc() gets tricky.

@mcspr mcspr merged commit c517bfd into esp8266:master Apr 28, 2023
@mhightower83 mhightower83 deleted the pr-fix-umm_poison branch June 16, 2023 17:24
mhightower83 added a commit to mhightower83/Arduino that referenced this pull request Jul 8, 2023
Bug introduced with PR fix esp8266#8914.
When a reallocated pointer could not grow in place, a replacement
allocation was created. Then UMM_POISON was written to the wrong block.
mhightower83 added a commit to mhightower83/Arduino that referenced this pull request Jul 9, 2023
d-a-v pushed a commit that referenced this pull request Jul 18, 2023
* Fixes occasional UMM_POISON failure

Bug introduced with PR fix #8914.
When a reallocated pointer could not grow in place, a replacement
allocation was created. Then UMM_POISON was written to the wrong block.

* Fix umm_poison data corruption on realloc when memory move is used.

Bug introduced with PR fix #8914

* refactored to resolve unused error in some build contexts
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
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
esp8266#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.
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* Fixes occasional UMM_POISON failure

Bug introduced with PR fix esp8266#8914.
When a reallocated pointer could not grow in place, a replacement
allocation was created. Then UMM_POISON was written to the wrong block.

* Fix umm_poison data corruption on realloc when memory move is used.

Bug introduced with PR fix esp8266#8914

* refactored to resolve unused error in some build contexts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Poison after the block Error in Smartconfig
2 participants