On Wed, Feb 03, 2021 at 08:30:42PM +0100, Marcus Glocker wrote:
> On Sun, Jan 31, 2021 at 07:05:29PM +0100, Thomas Jeunet wrote:
>
> > Hello tech,
> >
> > in ugen_set_config, the cached config descriptor (ugen.c:213) is
> > obsolete after the call to usbd_set_config_no (ugen.c:220). Use a
> > refreshed value so the next loop account for the correct number of
> > interfaces.
> >
> > I also believe the dev->cdesc is leaked inside usbd_set_config_index but
> > I haven't yet fully audited how the variable is used.
>
> Hmm, I don't think this is the right place to call
> usbd_get_config_descriptor() again. We only enter the if branch (line
> 214) to usbd_set_config_no() when there was a problem with the first
> usbd_get_config_descriptor() call:
>
> cdesc = usbd_get_config_descriptor(dev);
> if (cdesc == NULL || cdesc->bConfigurationValue != configno) {
>
> If there was no problem we skip that, and cdesc is still valid.
> Therefore I think usbd_get_config_descriptor() should be only called
> right after usbd_set_config_no().
>
Indeed, your diff looks better and also handle the case when
usbd_set_config_no fail. Thanks!
(And no memory leak, I missed a call to free...)
> Index: dev/usb/ugen.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ugen.c,v
> retrieving revision 1.114
> diff -u -p -u -p -r1.114 ugen.c
> --- dev/usb/ugen.c 1 Feb 2021 09:21:51 -0000 1.114
> +++ dev/usb/ugen.c 3 Feb 2021 19:27:37 -0000
> @@ -220,6 +220,10 @@ ugen_set_config(struct ugen_softc *sc, i
> err = usbd_set_config_no(dev, configno, 1);
> if (err)
> return (err);
> + cdesc = usbd_get_config_descriptor(dev);
> + if (cdesc == NULL ||
> + cdesc->bConfigurationValue != configno)
> + return (USBD_INVAL);
> }
> }
>
