Re: [PATCH QEMU HCI-8.0 v8 01/14] usb-host: don't leak hostdev FD on open failure #VSTOR-137800

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb-host: usb_host_open() fail path may double-close wrapped hostfd after libusb_close()
--

commit e4ff0c7f70381f2bead74b81a45f4480bcb0c76b
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

usb-host: don't leak hostdev FD on open failure #VSTOR-137800

This patch stores the caller-supplied hostfd in s->hostfd before
libusb_wrap_sys_device() is called. It also closes that fd on the fail:
path of usb_host_open(), so a failed open no longer leaks it.

> diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
> --- a/hw/usb/host-libusb.c
> +++ b/hw/usb/host-libusb.c

[ ... ]

> @@ -1066,6 +1070,10 @@ fail:
>          s->dh = NULL;
>          s->dev = NULL;
>      }
> +    if (s->hostfd != -1) {
> +        close(s->hostfd);
> +        s->hostfd = -1;
> +    }
>      return -1;
>  }

[Severity: Low]
Can this close() be a double close of the wrapped fd?

Consider the path where usb_host_open() is called with dev == NULL,
libusb_wrap_sys_device() succeeds, and a later step such as
usb_device_attach() fails. Control reaches fail: and runs:

    libusb_close(s->dh);
    s->dh = NULL;
    s->dev = NULL;
}
if (s->hostfd != -1) {
    close(s->hostfd);

Because s->hostfd is now set before the wrap call, this close() runs
right after libusb_close(s->dh).

If the libusb version in use closes a wrapped fd in libusb_close(), the fd
is closed twice. The second close() could then hit an unrelated descriptor
if another thread reused the number in between.

I could not check this against libusb sources. Current Linux backends mark
wrapped fds with fd_keep and do not close them, but older releases such as
1.0.23 may behave differently.

usb_host_close() already uses the same libusb_close() then close() order,
so the new error path relies on the same assumption. Is that assumption
valid for all libusb versions that QEMU supports?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-usb-host_v3-v8-0-72ae4fb8977b@virtuozzo.com?part=1