libusb_get_device_list returned incorrect old cached information on Windows

[udoudou]

On Windows, for a libusb_device* pointer a (device A) that has already been opened or refed by libusb_open or libusb_ref_device, if device A is unplugged and plugged in again, using libusb_get_device_list to retrieve the device list will still return the old cached libusb_device* pointer a, which will still record the previous device address.

[Youw]

🤖 Automated review by Claude — this review and its inline suggestion blocks were generated by Claude (claude-opus-4-8, via Claude Code) at a maintainer's request. Findings are advisory; please verify before acting. The suggestions are mechanical and have not been tested on hardware.

Summary

The intent is right and the eviction plumbing is correct, but as written the fix likely doesn't do what it claims — it compares two different quantities — and it adds root-hub churn.

🔴 Blocker — SPDRP_ADDRESS is a port number (per this file's own docs), not the USB device address

The new check evicts the cached device when dev->device_address != (uint8_t)SPDRP_ADDRESS, but those are different values:

• dev->device_address is set at windows_winusb.c:1208 from conn_info.DeviceAddress (IOCTL_USB_GET_NODE_CONNECTION_INFORMATION_EX on the parent hub) — the USB bus address (1–127), reassigned on every enumeration. That is what goes stale.
• SPDRP_ADDRESS is what this file's own get_dev_port_number() uses as a port-number fallback (:1288): "the address may be the port number, which is true for the Microsoft driver."

Consequences:

• On a same-port replug — the exact scenario (same DevInst/session_id, so the existing dev_id check at :1947 doesn't fire) — SPDRP_ADDRESS (the port) is unchanged, so the new check never fires and the stale device isn't evicted. The reported bug isn't actually fixed.
• Where a device's address ≠ its port number, the check can fire on every scan with no replug, churning the device and emitting spurious DEVICE_LEFT/arrived hotplug events.
• A different-port replug is already handled by the existing dev_id instance-ID check, so the new check is redundant there.

Recommendation: to detect a changed USB address, re-derive it from the same source the cache uses — IOCTL_USB_GET_NODE_CONNECTION_INFORMATION_EX.DeviceAddress — not SPDRP_ADDRESS. (Detail in the inline comment.)

🟠 Root hubs churn every scan

Root hubs get priv->initialized = true (:1230) but never get a device_address (the :1208 assignment is only on the parent_dev != NULL path), so it stays 0. The new block then compares 0 != SPDRP_ADDRESS(non-zero) → detach + unref + re-allocate every root hub on every enumeration. The inline suggestion mitigates this.

✅ What's correct

• The usbi_detach_device + libusb_unref_device + goto alloc_device mechanism is refcount-balanced and memory-safe — a faithful copy of the existing dev_id/class_guid eviction blocks (no double-free / leak / use-after-free).
• The SetupAPI call is well-formed (NULL reg-type, buffer/length, size == sizeof short-read guard, safe DWORD→uint8_t).

Nits

Missing space in )){; the "equivalent do" typo (copied from the existing blocks); the failure usbi_warn says "address" but reads the port property.

Inline comments carry one-click suggestions for the mechanical items (root-hub guard, brace, typo). The blocker is a design issue and needs a rethink rather than a one-line patch.

— Review generated by Claude (claude-opus-4-8)

[Youw]

🤖 Re-review by Claude — this follow-up review and its inline suggestion block were generated by Claude (claude-opus-4-8, via Claude Code) at a maintainer's request. Findings are advisory and have not been hardware-tested.

Thanks for the rework — the new get_dev_addr() helper is the right fix. Re-reviewed against d94da8b1:

✅ Resolved

• 🔴 Blocker (comparing the wrong quantity) — fixed. get_dev_addr() opens the parent hub and queries IOCTL_USB_GET_NODE_CONNECTION_INFORMATION_EX, comparing the real conn_info.DeviceAddress against the cached dev->device_address — i.e. the actual USB bus address the cache stored at allocation, so a same-port replug is now detected. This is the approach suggested in the original review.
• Root-hub churn — fixed. The check is gated on parent_dev, and get_dev_addr() returns false for a NULL parent, so root hubs (which never get a non-zero address) no longer detach/re-allocate on every scan.
• Typo equivalent do → to — fixed in all three blocks.
• Brace-spacing nit — obsolete (the SPDRP_ADDRESS line is gone).

The helper correctly mirrors the existing allocation-path IOCTL usage (:1137–1152); parent_priv->apib is always non-NULL (defaults to USB_API_UNSUPPORTED), so the != USB_API_HUB guard is safe; and the block only runs in the HUB/GEN passes where the parent is always a hub, so that guard won't misfire.

🟡 One new consideration (not a blocker) — inline on :1999: treating any get_dev_addr() failure as "address changed" can evict a still-present device on a transient hub-open failure, firing spurious DEVICE_LEFT/DEVICE_ARRIVED. Suggest evicting only on a positive mismatch. Plus one trivial style nit inline.

[Youw]

🤖 Final review round by Claude — generated by Claude (claude-opus-4-8, via Claude Code) at a maintainer's request. Advisory and not hardware-tested on my end; @udoudou verified the Windows behavior on Win11 25H2.

Re-reviewed against 72449a57. Everything raised across the previous rounds is now addressed, and the implementation looks correct.

Resolved this round

• Transient-failure churn (:1999) — now evicts only on a positive mismatch (get_dev_addr(...) && device_address != dev->device_address). @udoudou's Win11 25H2 testing shows this is also more correct than I'd assumed: a lost hub / empty port doesn't make the IOCTL fail — it returns success with DeviceAddress == 0. So the two cases separate cleanly:

  • device genuinely gone → get_dev_addr() succeeds with addr 0 → 0 != cached_addr → evicted ✅ (cached non-root devices always have a non-zero address, so removal is still reliably detected);
  • transient hub-open/contention failure → get_dev_addr() returns false → device kept, no phantom DEVICE_LEFT ✅.

  The two // ... (tested on Win11 25H2) comments documenting this are a nice touch.

• // comment spacing (:1304) — fixed.

From earlier rounds (still good)

• 🔴 wrong-quantity blocker → fixed via the parent-hub IOCTL_USB_GET_NODE_CONNECTION_INFORMATION_EX;
• root-hub churn → gated on parent_dev;
• equivalent do→to typos / brace nit → fixed / obsolete.

No new issues. One inherent, accepted trade-off worth stating once: each cached non-root device now costs a hub-open + IOCTL per libusb_get_device_list() scan (previously just string compares). That's the necessary price of detecting a same-port replug and it mirrors the allocation path's existing per-device cost, so it's fine.

LGTM on the logic. Since I can't exercise the hardware, I'd defer the final functional sign-off to your Win11 testing — but from a code-correctness standpoint this is good to merge.

— review by Claude (claude-opus-4-8), via Claude Code