Skip to content

fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] (IEC-434) #321

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] (IEC-434) #321

wants to merge 3 commits into from
+123 −53

Conversation

@roma-jam
Copy link
Contributor

@roma-jam roma-jam commented Nov 13, 2025 •
edited
Loading

Description

Fixed potential race condition in hid_host_device_close() that could lead to double-free and list corruption under concurrent close/disconnect.

Additional notes

Related

  • N/A

Testing

  • TBC

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@roma-jam
fix(usb_host_hid): Changed the handling of reallocating ctrl_xfer buf…
54c89ff 
…fer on large report descriptors

- Made all the null checks before any dereference
- Fixed the unlock path on usb_host_transfer_alloc() error
- Clamped req->wLength to some sane maximum (2048 bytes)
@roma-jam
fix(usb_host_hid): Harden hid_host_device_close() against concurrent …
87a6fec 
…close/disconnect
@roma-jam roma-jam self-assigned this Nov 13, 2025
@roma-jam roma-jam changed the title fix(usb_host_hid): Harden hid_host_device_close() against concurrent access fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] Nov 13, 2025
tore-espressif
tore-espressif reviewed Nov 14, 2025
View reviewed changes
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

Please consider adding tests

host/class/hid/usb_host_hid/hid_host.c
} else if ((hid_iface->state == HID_INTERFACE_STATE_WAIT_USER_DELETION ||
hid_iface->user_cb == NULL) &&
hid_iface->state != HID_INTERFACE_STATE_NOT_INITIALIZED) {
// Second close OR no user callback at all AND not already removed: remove from list
Copy link
Collaborator

@tore-espressif tore-espressif Nov 14, 2025

Choose a reason for hiding this comment

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

I can see that the '2 calls to close' was present even before. Can you explain why?

Copy link
Contributor Author

@roma-jam roma-jam Nov 14, 2025

Choose a reason for hiding this comment

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

Thanks for taking a look! This is still Draft / [WIP]. I’m landing a minimal bug fix first and prepared a draft as we discussed yesterday; if needed, I’ll follow up with a separate refactor to simplify the close logic.

On the “two calls to close”: yes, that existed before. When the upper layer opens the interface, it owns a handle. The first call closes the interface; the second removes the interface from the list and invalidates the handle so the upper layer never holds a dangling reference.

I’m keeping that behavior unchanged here to keep the fix small and low-risk. If you see anything blocking for the bug-fix itself, please feel free to share; otherwise I’ll ping when it’s ready for full review.

@roma-jam roma-jam added the Component: usb_host Issue affects usb_host component label Nov 14, 2025
@roma-jam roma-jam force-pushed the fix/report_descriptor_pointer_usage_on_realloc branch 4 times, most recently from 038e245 to e5e6f2d Compare November 20, 2025 11:54
@roma-jam roma-jam mentioned this pull request Nov 20, 2025
Merged
10 tasks
@roma-jam roma-jam force-pushed the fix/report_descriptor_pointer_usage_on_realloc branch 6 times, most recently from 1aafd73 to 81b37c9 Compare November 25, 2025 15:11
Base automatically changed from fix/report_descriptor_pointer_usage_on_realloc to master November 26, 2025 16:12
@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 28, 2025
@github-actions github-actions bot changed the title fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] fix(usb_host_hid): Harden hid_host_device_close() against concurrent access [WIP] (IEC-434) Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tore-espressif tore-espressif tore-espressif left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@roma-jam roma-jam

Labels

Component: usb_host Issue affects usb_host component Status: Opened Issue is new

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@roma-jam @tore-espressif @espressif-bot