_open_vc_itf parsing error when vc->std.bNumEndpoints != 0

[YixingShen]

The Hierarchy and Memory Layout
Standard Interface Descriptor (VC)
Video Control Header Descriptor: Contains wTotalLength.
Video Control Camera Terminal Descriptor
Video Control Output Terminal Descriptor
Standard Endpoint Descriptor: The physical interrupt pipe (Length = 7 bytes).
Class-Specific VC Interrupt Endpoint Descriptor: Additional endpoint metadata (Length = 5 bytes).

_end_of_control_descriptor is Output Terminal Descriptor End
the "end" should +7 for _find_desc searching Standard Endpoint Descriptor when vc->std.bNumEndpoints != 0

[Copilot]

Pull request overview

Fixes UVC VideoControl (VC) interface parsing when vc->std.bNumEndpoints != 0 by ensuring the endpoint descriptor is included in the search window derived from wTotalLength.

Changes:

• Extends the computed “end of VC descriptor” pointer so _find_desc(..., TUSB_DESC_ENDPOINT) can find the notification endpoint descriptor.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Extending end by a hard-coded 7 bytes inside _open_vc_itf() fixes the endpoint search, but the same endpoint lookup pattern in _close_vc_itf() still uses _end_of_control_descriptor() without extending the search range and will return end/assert when bNumEndpoints != 0. Consider introducing a helper that computes the true end of the VC interface including the notification endpoint descriptor(s) (only when bNumEndpoints is non-zero), and use it in both open/close so behavior stays consistent.

[hathach]

@claude review this

[claude]

Claude finished @hathach's task in 1m 35s —— View job

---

Code Review

• Gather context
• Read changed file and diff
• Analyze the fix
• Post review feedback

---

Bug Analysis

The fix addresses a real bug. _end_of_control_descriptor() computes:

return ((uint8_t const*) desc) + vc->std.bLength + tu_le16toh(vc->ctl.wTotalLength);

Per the UVC spec, wTotalLength covers only the class-specific VC interface descriptors (Header + Camera Terminal + Output Terminal). The Standard Endpoint Descriptor (7 bytes) and Class-Specific VC Interrupt Endpoint Descriptor (5 bytes) live outside that range. So when bNumEndpoints != 0, _find_desc(cur, end, TUSB_DESC_ENDPOINT) searches [cur, end) where end is exactly at the start of the endpoint descriptor—it can never be found. The PR is correct in principle.

---

Issues with the Current Fix

1. Pointer arithmetic on void const* (build error)

// As submitted:
end = _end_of_control_descriptor(cur) + 7;

_end_of_control_descriptor() returns void const*. Arithmetic on void* is undefined by the C standard—GCC's -Wpointer-arith flag (which TinyUSB builds with -Wall -Werror) will reject this and fail the build. It must be cast first:

end = (uint8_t const *)_end_of_control_descriptor(cur) + sizeof(tusb_desc_endpoint_t);

2. Magic number 7

Use sizeof(tusb_desc_endpoint_t) instead of a bare 7. It is self-documenting and guards against future struct changes.

3. _close_vc_itf() has the same bug (incomplete fix)

_open_vc_itf is fixed, but _close_vc_itf() at line 741-744 has identical logic and is not fixed:

// _close_vc_itf() – still broken when bNumEndpoints != 0
void const *end = _end_of_control_descriptor(vc);          // line 741
...
cur = _find_desc(cur, end, TUSB_DESC_ENDPOINT);             // line 744
TU_ASSERT(cur < end);                                       // will fire!

With bNumEndpoints != 0, this TU_ASSERT will trigger because the endpoint descriptor is outside [cur, end), causing a failed close that leaves the endpoint open when alternate setting switching occurs.

4. Fragility of + sizeof(tusb_desc_endpoint_t)

Extending end by exactly one tusb_desc_endpoint_t works only when bNumEndpoints == 1 (which is verified by the TU_VERIFY(1 == vc->std.bNumEndpoints) guard in _open_vc_itf, but there is no such guard in _close_vc_itf). A more robust approach is a small helper:

static inline void const* _end_of_vc_descriptor(void const *desc)
{
  tusb_desc_vc_itf_t const *vc = (tusb_desc_vc_itf_t const *)desc;
  uint8_t const *end = (uint8_t const*)desc + vc->std.bLength
                       + tu_le16toh(vc->ctl.wTotalLength);
  if (vc->std.bNumEndpoints) {
    end += sizeof(tusb_desc_endpoint_t);  // standard EP descriptor
    end += 5;                             // class-specific EP descriptor (fixed 5 bytes per UVC spec)
  }
  return end;
}

This keeps both _open_vc_itf and _close_vc_itf consistent without duplicating the offset logic.

---

Summary

	Status
Root cause identified correctly	✅
Fix logic is correct	✅
Build-safe (no void* arithmetic)	❌
Uses named constant instead of magic 7	❌
_close_vc_itf() also fixed	❌

The core diagnosis is right, but the patch will not build as submitted and leaves _close_vc_itf() broken. Please address the three issues above before merging.