fix(modem): UartTerminal::on_read usage/set data race - #1112
Conversation
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
UartTerminal::set_read_cb can be called between the check and call of on_read in UartTerminal::task triggering a data race between them resulting in a segfault. This commit fixes this by adding a lock for the use of on_read
3b81768 to
cebfab0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cebfab0. Configure here.
| Scoped<Lock> lock(on_read_lock); | ||
| if (len && on_read) { | ||
| on_read(nullptr, len); | ||
| } |
There was a problem hiding this comment.
Callback destroyed during nested set
Medium Severity
The new recursive on_read_lock is documented to allow on_read to call set_read_cb and replace the callback while it runs, but the task still invokes on_read directly. A nested set_read_cb assigns over that same std::function, destroying the callable mid-invocation. That path is used by DTE::on_read when a reply completes, and can cause heap corruption or crashes.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit cebfab0. Configure here.
|
Hi @markxoe Thank you for the contribution with your clean and minimal patch to modem sources! |
|
Hi @david-cermak that looks like the better solution, thanks for merging it so quickly! |
4 participants
Description
While testing esp_modem (v2.0.2) with a SIM7080 and ESP32-S3 I reproducibly got several crashes while calling
esp_modem_set_mode. The stack traces showed either double-free or segfault. Further debugging withCONFIG_HEAP_POISONING_COMPREHENSIVEnarrowed the problem down to thison_read()callback being invoked even after it had already been set tonullptr.The problem seems to originate from a data race where
set_read_cbis called between the check before usage ofon_readand the call ofon_read.This PR fixes this issue by locking a mutex when
on_readis either set or used.Related
A quick search found #1014 to be related: this bug is marked as
UART-003but not fixed in the PR. I found no further mention of this bug.Testing
Only manual testing pre- and post-fixing were done due to the bug being complex to test. The bug has not yet occurred after the fix and thorough manual testing.
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Medium Risk
Touches concurrency on the UART RX hot path; incorrect locking could deadlock or stall modem I/O, but the change is narrowly scoped and matches existing esp_modem Lock primitives.
Overview
Fixes crashes during
esp_modem_set_modecaused by a race: the UART task could invokeon_readafter another thread cleared it viaset_read_cb.UartTerminal now holds a recursive
Lock(on_read_lock) declared before the UART task so it outlives the task.set_read_cbtakes the lock while swapping the callback; the UARTtaskloop takes the same lock around the null-check and invocation ofon_readon both theUART_DATApath and the polling timeout path. Recursion is required becauseon_readmay callset_read_cbre-entrantly.Reviewed by Cursor Bugbot for commit cebfab0. Bugbot is set up for automated code reviews on this repo. Configure here.