[snokvist]

Problem

sync_xfer_new() recycles libusb transfers from a per-device pool (since #444, b9b80de). A recycled transfer keeps its last actual_length, because libusb writes that field only when a transfer completes (usbi_handle_transfer_completion). It never writes it when libusb_submit_transfer() fails. submit_and_wait() returns a submit failure directly, and sync_bulk() then reads actual_length anyway. So once the device has left the bus, an IN read reports the previous transfer's length and memcpys that many bytes from its len-byte bounce buffer into the caller's buffer. That is a heap over-read plus an overflow of the caller's buffer.

Fix

One line: sync_xfer_new() zeroes actual_length on the transfer it hands out. Both sync helpers go through it.

Evidence

Reproduced on hardware (MT7612U, USB 3). The reproducer:

1. opens and starts the adapter;
2. sends one 1400 B frame through mt7612u_tx() (a sync bulk OUT, 1428 B on the wire);
3. deauthorizes the adapter in sysfs;
4. calls mt_bulk() on EP 0x85 with a 64 B length into a sentinel-filled buffer.

build	rc	n reported	bytes written past the 64 B buffer
master 34a9e6f	LIBUSB_ERROR_IO	1428	1364
this branch	LIBUSB_ERROR_IO	0	0

Worst real case: a TX of up to MT_TX_BUF_MAX (4096 B), then an unplug, then an MCU reply read into its 1024 B stack buffer (mcu.cpp).

Scope (from an adversarial review before opening, checked against libusb 1.0.29's io.c):

• libusb_alloc_transfer callocs, so a fresh transfer starts at 0.
• libusb_submit_transfer resets only the internal transferred count. After a successful submit, every completion (cancel, timeout and disconnect included) writes actual_length, never above length. So only a failed submit leaves it stale, and a clamp in sync_bulk() would add nothing.
• sync_control() reads actual_length only when rc == 0 and was not affected.
• The async RX/TX rings use their own transfers and read the field only in completion callbacks.
• Nothing else in the repo pools libusb transfers. UsbTransport resets its slot's length on hand-out.

Limits:

• One unit.
• The reproducer used a sysfs deauthorize, which fails the submit with LIBUSB_ERROR_IO. A physical unplug gives LIBUSB_ERROR_NO_DEVICE, which takes the same path but was not run.
• The reproducer is a scratch program, not added to tests/: it needs the internal header and root.
• ctest passes 86/86 with -DDEVOURER_MT7612U=ON, and the fixed build brought the adapter up, started it and transmitted normally.

🤖 Generated with Claude Code

https://claude.ai/code/session_011dReg1L6xtBhf95NpAeuge

[qodo-free-for-open-source-projects]

PR Summary by Qodo

mt7612u: Clear stale transfer length before synchronous reuse

🐞 Bug fix 🕐 Less than 10 minutes

AI Description

• Reset pooled transfers’ reported length before reuse, so failed submissions report zero bytes.
• Prevent stale lengths from causing an IN read to copy beyond its buffers.

Diagram

graph TD
  A["Sync bulk"] --> B["Acquire transfer"] --> C["Reset length"] --> D{"Submit succeeds?"} -->|No| E["Zero bytes"] --> G["Caller buffer"]
  D -->|Yes| F["Completion length"] --> G

Loading

High-Level Assessment

Resetting actual_length when a transfer is handed out establishes a safe starting value for both synchronous helpers. Handling only the bulk error path would leave initialization dependent on individual callers.

Files changed (1) +8 / -0

Bug fix (1) +8 / -0

usb.cppInitialize the reported length of acquired transfers +8/-0

Initialize the reported length of acquired transfers

• sync_xfer_new() now clears actual_length after acquiring a transfer, including one reused from the device pool. If libusb submission fails without updating that field, sync_bulk() reports zero transferred bytes instead of copying data using a previous transfer’s length.

src/mt7612u/usb.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can route each severity your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

Review: PR 512

Verdict: approve. The diagnosis holds against the code: sync_bulk() reads actual_length on every return except the abandon path, submit_and_wait() returns a failed submit straight through, and a pooled transfer carries the previous completion's length. Zeroing it at hand-out in sync_xfer_new() is the right place, since both sync helpers go through it and nothing else pools transfers.

Checked here:

• Built the branch with -DDEVOURER_MT7612U=ON: no warnings in src/mt7612u/, ctest 86/86.
• sync_control() is unaffected as stated (it reads the field only when rc == 0).

Not checked: the hardware reproducer. The bench's MT7612U (slot #17) is unplugged today, so the deauthorize-then-read case is the author's measurement only.

Gate: qodo-gate is red because Qodo posted its "no issues found" as an issue comment, not a review object, so the check sees no review. That is the gate's known shape for a clean verdict; the skip-qodo-gate label is the way through.