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

esp_event: fix crash when unregistering a handler instance in itself (IDFGH-1940) #4139

Closed
wants to merge 1 commit into from

Conversation

xentec
Copy link
Contributor

@xentec xentec commented Sep 30, 2019

When a handler instance is the last one in the list und unregisters itself, the handler iterator will be invalidated by entering free'd memory. Same applies for event base and id, if they become empty.

When a handler instance is the last one in the list und unregisters
itself, the handler iterator will be invalidated by entering free'd
memory. Same applies for event base and id, if they become empty.
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot changed the title esp_event: fix crash when unregistering a handler instance in itself esp_event: fix crash when unregistering a handler instance in itself (IDFGH-1940) Oct 1, 2019
esp_event_base_node_t *base_node;
esp_event_id_node_t *id_node;
esp_event_base_node_t *base_node, *temp_base;
esp_event_id_node_t *id_node, *temp_id_node;

SLIST_FOREACH(loop_node, &(loop->loop_nodes), next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to also need the safe foreach variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the long silence, but you're right. I've also seen your fixup.

igrr pushed a commit to igrr/esp-idf that referenced this pull request Nov 4, 2019
When a handler instance is the last one in the list und unregisters
itself, the handler iterator will be invalidated by entering free'd
memory. Same applies for event base and id, if they become empty.

Merges espressif#4139
@xentec
Copy link
Contributor Author

xentec commented Nov 18, 2019

After further debugging, I've detected a heap corruption when using event profiling. When unregistering event handlers in themselves, the handler instance is free'd during execution, but its fields are still written to from profiling code afterwards:

(*(handler->handler))(handler->arg, post.base, post.id, post.data);
#ifdef CONFIG_EVENT_LOOP_PROFILING
diff = esp_timer_get_time() - start;
xSemaphoreTake(loop->profiling_mutex, portMAX_DELAY);
handler->invoked++;
handler->time += diff;

A possible solution is to move handler instances to special list for deallocation after their deregistration and free them altogether after event execution is done.

@renzbagaporo
Copy link
Contributor

renzbagaporo commented Mar 16, 2020

Hi @xentec, can you open a new issue to track this? I only noticed your reply now. Assign the issue to me if Github allows it (I'm not sure).

espressif-bot pushed a commit that referenced this pull request Mar 27, 2020
When a handler instance is the last one in the list und unregisters
itself, the handler iterator will be invalidated by entering free'd
memory. Same applies for event base and id, if they become empty.

Merges #4139
espressif-bot pushed a commit that referenced this pull request Apr 13, 2020
When a handler instance is the last one in the list und unregisters
itself, the handler iterator will be invalidated by entering free'd
memory. Same applies for event base and id, if they become empty.

Merges #4139
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.

3 participants