On Thu, Feb 04, 2021 at 10:28:42PM +0100, Thomas Jeunet wrote:
> 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!
Ok, thanks for reporting this. I'll seek out for an OK and once
received commit it.
> (And no memory leak, I missed a call to free...)
Good! :-)
> > 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);
> > }
> > }
> >
