Skip to content

Conversation

@roma-jam
Copy link
Collaborator

@roma-jam roma-jam commented Nov 13, 2025

Description

Main change

  • Fixed the ctrl_xfer local pointer usage

Additional clean-up

  • 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

Related

  • N/A

Testing

  • N/A

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 roma-jam self-assigned this Nov 13, 2025
…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 roma-jam force-pushed the fix/report_descriptor_pointer_usage_on_realloc branch from 5913c4c to 54c89ff Compare November 13, 2025 11:40
@roma-jam roma-jam marked this pull request as ready for review November 13, 2025 13:04
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.

LGTM.

Please consider adding tests for this situation

if (ctrl_xfer->actual_num_bytes < USB_SETUP_PACKET_SIZE) {
ret = ESP_ERR_INVALID_SIZE;
} else {
ctrl_xfer->actual_num_bytes -= USB_SETUP_PACKET_SIZE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: actual_num_bytes should be modified by the Host driver only.

I suggest adding intermediate variable for clarity

const size_t hid_report_len = ctrl_xfer->actual_num_bytes - USB_SETUP_PACKET_SIZE;

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