Skip to content

hotplug: fix callback use-after-free races #1861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smaeljaish771 wants to merge 2 commits into
base: master
Choose a base branch
from
+29 −5
Open

hotplug: fix callback use-after-free races #1861

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4024cb
hotplug: fix callback use-after-free races
yirutx Jun 26, 2026
fbaa954
address review comments
yirutx Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions libusb/hotplug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,12 @@ void usbi_hotplug_exit(struct libusb_context *ctx)
return;

/* free all registered hotplug callbacks */
usbi_mutex_lock(&ctx->hotplug_cbs_lock);
for_each_hotplug_cb_safe(ctx, hotplug_cb, next_cb) {
list_del(&hotplug_cb->list);
free(hotplug_cb);
}
usbi_mutex_unlock(&ctx->hotplug_cbs_lock);

/* free all pending hotplug messages */
while (!list_empty(&ctx->hotplug_msgs)) {
Expand Down Expand Up @@ -348,6 +350,7 @@ int API_EXPORTED libusb_hotplug_register_callback(libusb_context *ctx,
libusb_hotplug_callback_handle *callback_handle)
{
struct usbi_hotplug_callback *hotplug_cb;
libusb_hotplug_callback_handle handle;

/* check for sane values */
if (!events || (~VALID_HOTPLUG_EVENTS & events) ||
Expand Down Expand Up @@ -389,6 +392,7 @@ int API_EXPORTED libusb_hotplug_register_callback(libusb_context *ctx,

/* protect the handle by the context hotplug lock */
hotplug_cb->handle = ctx->next_hotplug_cb_handle++;
handle = hotplug_cb->handle;

/* handle the unlikely case of overflow */
if (ctx->next_hotplug_cb_handle < 0)
Expand All @@ -399,29 +403,47 @@ int API_EXPORTED libusb_hotplug_register_callback(libusb_context *ctx,
usbi_mutex_unlock(&ctx->hotplug_cbs_lock);

usbi_dbg(ctx, "new hotplug cb %p with handle %d",
(void *) hotplug_cb, hotplug_cb->handle);
(void *) hotplug_cb, handle);

if ((flags & LIBUSB_HOTPLUG_ENUMERATE) && (events & LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED)) {
ssize_t i, len;
struct libusb_device **devs;

len = libusb_get_device_list(ctx, &devs);
if (len < 0) {
libusb_hotplug_deregister_callback(ctx, hotplug_cb->handle);
libusb_hotplug_deregister_callback(ctx, handle);
return (int)len;
}

for (i = 0; i < len; i++) {
struct usbi_hotplug_callback hotplug_cb_copy;
int found = 0;

usbi_mutex_lock(&ctx->hotplug_cbs_lock);
for_each_hotplug_cb(ctx, hotplug_cb) {

@sonatique sonatique Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the lookup reuses the outer hotplug_cb as its iterator. It's safe (unused afterward), but maybe not explicit enough when reading the code, and maybe not super-safe against refactoring.
What about using a dedicated value?

@smaeljaish771 smaeljaish771 Jun 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Documented the ENUMERATE deregistration behavior and used a dedicated iterator.

if (handle == hotplug_cb->handle) {
if (!(hotplug_cb->flags & USBI_HOTPLUG_NEEDS_FREE)) {
hotplug_cb_copy = *hotplug_cb;
found = 1;
}
break;
}
}
usbi_mutex_unlock(&ctx->hotplug_cbs_lock);

if (!found)
break;

usbi_hotplug_match_cb(devs[i],
LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED,
hotplug_cb);
&hotplug_cb_copy);
}

libusb_free_device_list(devs, 1);
}

if (callback_handle)
*callback_handle = hotplug_cb->handle;
*callback_handle = handle;

return LIBUSB_SUCCESS;
}
Expand Down
Loading