darwin: keep the parent device link correct across re-enumeration - #1917
darwin: keep the parent device link correct across re-enumeration#1917sonatique wants to merge 2 commits into
Conversation
darwin_get_cached_device assigns parent_session on every path, not only when it allocates a cached device, so a device returning from a re-enumeration carries the session id of the parent it currently sits behind. A parent hub that is itself re-enumerated receives a new session id. Its children now resolve that new id the next time they are re-enumerated, so libusb_get_parent keeps returning the parent and libusb_get_port_numbers keeps reporting the full path. Reference: libusb#1798
process_new_device resolves the new parent and stores it into dev->parent_dev before releasing the previous one, so the field always holds either NULL or a device this device owns a reference to. The device being updated is already visible to the application, and libusb_get_port_numbers walks the parent chain without a lock. A concurrent walk therefore sees one parent or the other for the whole update, with no NULL gap while the lookup runs and no pointer to a device that has been freed. That matters because the reader measures the chain depth and fills the caller's array in two separate passes, and a chain that changes length between them yields a result that does not match what was measured. Where the parent is unchanged the stored value is identical, so no transition is observable at all. Reference counting stays balanced: usbi_get_device_by_session_id returns a referenced device, so this is the same +1/-1 pair in the opposite order. The window is narrowed rather than closed. A walk that loaded the old pointer before the store can still dereference it after the release; closing that requires the reader to hold a reference, which is a core-level change. Reference: libusb#1798 (addendum item 12)
|
@sonatique I assume this was (at least partially) generated by AI? or was this all you? |
@Youw: the code (2 commits) is mine, but indeed the PR description is from Claude, I was working concurrently on this PR and the more complicated #1919. I had these commits done on one branch and started the other one on top, but I finally decided to rely on Claude for #1919 because I was getting lost. When done, I re-separated the commits to make 2 PRs and asked Claude to make the PR description for that one too, out of laziness. Should I add a line saying the description is AI generated? |
That is still an open question. For the most part people mostly care about the code being "not poluted by AI bugs". When I see this much of the text in PR description, I automatically assume human is too lazy to write it, so it must be an AI. But then, if AI has written the description, maybe the code too. But it is not the case this time... Maybe we should extend our |
|
Codex reviewed both commits at
Generated by Codex (GPT-5.6 Sol). |
The problem is: these AIs are always so verbose, reading everything they output takes so much time. "If you're too lazy to write it, then I'm too lazy to read it" is a sentiment I'm seeing more and more, and I'm starting to appreciate it. It's easy to generate tonnes and tonnes of AI text, but hard and slow to read it all. (I'm not trying to pick on you here, this is happening everywhere.) |
There is two sides to the coin to this. I agree by default it generates tons of code and reading it by a human usually exhausting. This actually can easily be adjusted by giving a corresponding AGENTS.md directive. On the other hand, I usually use other agents to read/analyze/fix and give me a short summary, and in such cases - the more context provided for input - the higher chance to get an accurate secondary analysys. |
|
Let me test this PR over the weekend. |
|
First test -- |
4 participants
Two fixes in the darwin re-enumeration path, found while investigating the
libusb_get_port_numbers/process_new_devicereport on #1798. They are differentkinds of fix in the same code, so they are separate commits. A companion fix bounding
the reader's fill loop is in PR#1915.
darwin: refresh the cached parent session on re-enumeration(functional, noconcurrency involved)
darwin_get_cached_deviceassignsparent_sessionon every path, not only when itallocates a cached device, so a device returning from a re-enumeration carries the
session id of the parent it currently sits behind. When a parent hub is re-enumerated
and receives a new session id, its children resolve that new id the next time they are
re-enumerated, and
libusb_get_parentandlibusb_get_port_numberskeep reporting thereal topology.
darwin: publish the new parent device before releasing the old one(addendum item12 of #1798)
process_new_deviceresolves the new parent and stores it before releasing the previousone, so
dev->parent_devalways holds either NULL or a device this device owns areference to. A concurrent
libusb_get_port_numberswalk sees one parent or the other,with no NULL gap while the lookup runs. Reference counting stays balanced:
usbi_get_device_by_session_idreturns a referenced device, so this is the same +1/-1pair in the opposite order.
It narrows the reader race rather than closing it. A walk that already loaded the old
pointer can still dereference it after the release, and closing that needs the reader to
hold a reference, which is a core-level change.
Reference: #1798 (addendum item 12)
Regarding testing:
The first commit has a visible symptom and is the one worth exercising: with a device
behind a hub, replug or re-enumerate the hub, then reset the child, and check that
libusb_get_parentand the port path still resolve.hotplugtestplustestlibusbcovers it.
The second has no targeted reproducer, so the usual suite for no-regression
is enough.
If somebody re-runs the item-9/10 tester (its scan thread calls
libusb_get_port_numbers),the TSan report on
parent_devstill appears. It flags the unlocked read of the field,which neither commit changes; the recent clean runs used tests that never walk the
parent chain, so they did not exercise it. Silencing it needs the field made atomic, or
both the reader and the backend writer holding a common lock, with
ctx->usb_devs_lockthe natural candidate since
usbi_hotplug_exitalready walks parent chains under it.Both are core changes, and separate from the use-after-free question above.