fix(usb_host): Give semaphore on attempted close of non-opened device (IDFGH-14893) #15608
Conversation
👋 Hello mykmelez, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
If you call *usb_host_device_close()* for a device that isn't open, the function exits early,
without giving back the semaphore it took, which causes any other call that tries to take
that semaphore to hang indefinitely.
Strangely, there's redundant handling of this condition, with two checks in a row that both handle
the case where `_check_client_opened_device(client_obj, dev_addr)` returns `false`:
```c
HOST_CHECK_FROM_CRIT(_check_client_opened_device(client_obj, dev_addr), ESP_ERR_NOT_FOUND);
if (!_check_client_opened_device(client_obj, dev_addr)) {
// Client never opened this device
ret = ESP_ERR_INVALID_STATE;
HOST_EXIT_CRITICAL();
goto exit;
}
…
exit:
xSemaphoreGive(p_host_lib_obj->constant.mux_lock);
return ret;
```
The first line is the one that exits early, as HOST_CHECK_FROM_CRIT returns its second parameter
if its first parameter is false, without giving back the semaphore (although it does exit
the critical section).
The subsequent block handles the exact same case, except that it ensures the semaphore is given
back before returning. Currently, this block is never reached.
Perhaps the first check was added, then someone noticed the issue and added the second check,
but they forgot to remove the first one.
In any case, this PR removes the first check, so the second check can properly handle this case
by giving back the semaphore before returning.
This bug appears to have been present in the initial commit of the USB Host library to the ESP-IDF
repo: espressif@accbaee
Of course, if you never try to close a non-opened device, then you won't encounter it!
Unfortunately, I have some code that tried to do that, which is how I found the issue.
mykmelez
force-pushed
the
usb-host-device-close-err-give-semaphore
branch
from
1bc06ab to
f560629
Compare
mykmelez
changed the title
github-actions
bot
changed the title
|
@mykmelez Thank you very much for the fix! We will merge internally and backport to all active release branches |
espressif-bot
added
Status: Selected for Development
espressif-bot
added
Status: In Progress
|
sha=a3864c088dafb0b8ce94dba272685b850b46c837 |
|
Thanks again, the commit is already in master branch aa669fa The PR was not closed automatically, so closing now! |
espressif-bot
added
Status: Done
Description
If you call usb_host_device_close() for a device that isn't open, the function exits early, without giving back the semaphore it took, which causes any other call that tries to take that semaphore to hang indefinitely.
Strangely, there's redundant handling of this condition, with two checks in a row that both handle the case where
_check_client_opened_device(client_obj, dev_addr)returnsfalse:The first line is the one that exits early, as HOST_CHECK_FROM_CRIT returns its second parameter if its first parameter is false, without giving back the semaphore (although it does exit the critical section).
The subsequent block handles the exact same case, except that it ensures the semaphore is given back before returning. Currently, this block is never reached.
Perhaps the first check was added, then someone noticed the issue and added the second check, but they forgot to remove the first one.
In any case, this PR removes the first check, so the second check can properly handle this case by giving back the semaphore before returning.
This bug appears to have been present in the initial commit of the USB Host library to the ESP-IDF repo: accbaee
Of course, if you never try to close a non-opened device, then you won't encounter it! Unfortunately, I have some code that tried to do that, which is how I found the issue.
Related
Testing
Before this change, I stepped through usb_host_device_close() in GDB to confirm that it was exiting early without giving back the semaphore when my code tried to close a device that wasn't open. After this change, I stepped through the function again to confirm that it was giving back the semaphore before exiting.
As well, before this change, I observed that threads that called functions that take the same semaphore, such as usb_host_client_register() and usb_host_client_deregister(), would hang indefinitely awaiting the semaphore. After this change, I observed that those threads didn't hang.
Checklist
Before submitting a Pull Request, please ensure the following: