umidi: missing NULL checks

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

umidi: missing NULL checks

Tobias Heider-2
In alloc_all_jacks() the variables 'sc_in_jacks' and 'sc_out_checks'
are set to NULL if 'sc_in_num_jacks' and 'sc_out_num_jacks' are 0.
Further down both are dereferenced unconditionally. I added explicit NULL
checks where I think they belong.
I think 'sc_in_ep' and 'sc_out_ep' can also be NULL, but I am not
entirely sure about those.

Objections or ok?

Index: dev/usb/umidi.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/dev/usb/umidi.c,v
retrieving revision 1.53
diff -u -p -r1.53 umidi.c
--- dev/usb/umidi.c 16 Mar 2020 16:12:43 -0000 1.53
+++ dev/usb/umidi.c 23 Mar 2020 21:50:49 -0000
@@ -724,50 +724,58 @@ alloc_all_jacks(struct umidi_softc *sc)
  sc->sc_in_jacks =
     sc->sc_in_num_jacks ? sc->sc_jacks+sc->sc_out_num_jacks : NULL;
 
- jack = &sc->sc_out_jacks[0];
- for (i=0; i<sc->sc_out_num_jacks; i++) {
- jack->opened = 0;
- jack->binded = 0;
- jack->arg = NULL;
- jack->u.out.intr = NULL;
- jack->intr = 0;
- jack->cable_number = i;
- jack++;
+ if (sc->sc_out_jacks) {
+ jack = &sc->sc_out_jacks[0];
+ for (i=0; i<sc->sc_out_num_jacks; i++) {
+ jack->opened = 0;
+ jack->binded = 0;
+ jack->arg = NULL;
+ jack->u.out.intr = NULL;
+ jack->intr = 0;
+ jack->cable_number = i;
+ jack++;
+ }
  }
- jack = &sc->sc_in_jacks[0];
- for (i=0; i<sc->sc_in_num_jacks; i++) {
- jack->opened = 0;
- jack->binded = 0;
- jack->arg = NULL;
- jack->u.in.intr = NULL;
- jack->cable_number = i;
- jack++;
+ if (sc->sc_in_jacks) {
+ jack = &sc->sc_in_jacks[0];
+ for (i=0; i<sc->sc_in_num_jacks; i++) {
+ jack->opened = 0;
+ jack->binded = 0;
+ jack->arg = NULL;
+ jack->u.in.intr = NULL;
+ jack->cable_number = i;
+ jack++;
+ }
  }
 
  /* assign each jacks to each endpoints */
- jack = &sc->sc_out_jacks[0];
- ep = &sc->sc_out_ep[0];
- for (i=0; i<sc->sc_out_num_endpoints; i++) {
- rjack = &ep->jacks[0];
- for (j=0; j<ep->num_jacks; j++) {
- *rjack = jack;
- jack->endpoint = ep;
- jack++;
- rjack++;
+ if (sc->sc_out_jacks && sc->sc_out_ep) {
+ jack = &sc->sc_out_jacks[0];
+ ep = &sc->sc_out_ep[0];
+ for (i=0; i<sc->sc_out_num_endpoints; i++) {
+ rjack = &ep->jacks[0];
+ for (j=0; j<ep->num_jacks; j++) {
+ *rjack = jack;
+ jack->endpoint = ep;
+ jack++;
+ rjack++;
+ }
+ ep++;
  }
- ep++;
  }
- jack = &sc->sc_in_jacks[0];
- ep = &sc->sc_in_ep[0];
- for (i=0; i<sc->sc_in_num_endpoints; i++) {
- rjack = &ep->jacks[0];
- for (j=0; j<ep->num_jacks; j++) {
- *rjack = jack;
- jack->endpoint = ep;
- jack++;
- rjack++;
+ if (sc->sc_in_jacks && sc->sc_in_ep) {
+ jack = &sc->sc_in_jacks[0];
+ ep = &sc->sc_in_ep[0];
+ for (i=0; i<sc->sc_in_num_endpoints; i++) {
+ rjack = &ep->jacks[0];
+ for (j=0; j<ep->num_jacks; j++) {
+ *rjack = jack;
+ jack->endpoint = ep;
+ jack++;
+ rjack++;
+ }
+ ep++;
  }
- ep++;
  }
 
  return USBD_NORMAL_COMPLETION;

Reply | Threaded
Open this post in threaded view
|

Re: umidi: missing NULL checks

Alexandre Ratchov-2
On Mon, Mar 23, 2020 at 10:51:06PM +0100, Tobias Heider wrote:
> In alloc_all_jacks() the variables 'sc_in_jacks' and 'sc_out_checks'
> are set to NULL if 'sc_in_num_jacks' and 'sc_out_num_jacks' are 0.
> Further down both are dereferenced unconditionally. I added explicit NULL
> checks where I think they belong.
> I think 'sc_in_ep' and 'sc_out_ep' can also be NULL, but I am not
> entirely sure about those.
>
> Objections or ok?
>

sc_{out,in}_jacks is used only if sc_out_num_jacks > 0, i.e. if
there's at least one iteration. But if sc_out_num_jacks > 0, then
sc_{out,in}_jacks is properly initialized by this chunk:

        sc->sc_out_jacks =
            sc->sc_out_num_jacks ? sc->sc_jacks : NULL;
        sc->sc_in_jacks =
            sc->sc_in_num_jacks ? sc->sc_jacks + sc->sc_out_num_jacks : NULL;

AFAICS, the code is correct; but the style looks misleading to me.

> Index: dev/usb/umidi.c
> ===================================================================
> RCS file: /mount/openbsd/cvs/src/sys/dev/usb/umidi.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 umidi.c
> --- dev/usb/umidi.c 16 Mar 2020 16:12:43 -0000 1.53
> +++ dev/usb/umidi.c 23 Mar 2020 21:50:49 -0000
> @@ -724,50 +724,58 @@ alloc_all_jacks(struct umidi_softc *sc)
>   sc->sc_in_jacks =
>      sc->sc_in_num_jacks ? sc->sc_jacks+sc->sc_out_num_jacks : NULL;
>  
> - jack = &sc->sc_out_jacks[0];
> - for (i=0; i<sc->sc_out_num_jacks; i++) {
> - jack->opened = 0;
> - jack->binded = 0;
> - jack->arg = NULL;
> - jack->u.out.intr = NULL;
> - jack->intr = 0;
> - jack->cable_number = i;
> - jack++;
> + if (sc->sc_out_jacks) {
> + jack = &sc->sc_out_jacks[0];
> + for (i=0; i<sc->sc_out_num_jacks; i++) {
> + jack->opened = 0;
> + jack->binded = 0;
> + jack->arg = NULL;
> + jack->u.out.intr = NULL;
> + jack->intr = 0;
> + jack->cable_number = i;
> + jack++;
> + }
>   }
> - jack = &sc->sc_in_jacks[0];
> - for (i=0; i<sc->sc_in_num_jacks; i++) {
> - jack->opened = 0;
> - jack->binded = 0;
> - jack->arg = NULL;
> - jack->u.in.intr = NULL;
> - jack->cable_number = i;
> - jack++;
> + if (sc->sc_in_jacks) {
> + jack = &sc->sc_in_jacks[0];
> + for (i=0; i<sc->sc_in_num_jacks; i++) {
> + jack->opened = 0;
> + jack->binded = 0;
> + jack->arg = NULL;
> + jack->u.in.intr = NULL;
> + jack->cable_number = i;
> + jack++;
> + }
>   }
>  
>   /* assign each jacks to each endpoints */
> - jack = &sc->sc_out_jacks[0];
> - ep = &sc->sc_out_ep[0];
> - for (i=0; i<sc->sc_out_num_endpoints; i++) {
> - rjack = &ep->jacks[0];
> - for (j=0; j<ep->num_jacks; j++) {
> - *rjack = jack;
> - jack->endpoint = ep;
> - jack++;
> - rjack++;
> + if (sc->sc_out_jacks && sc->sc_out_ep) {
> + jack = &sc->sc_out_jacks[0];
> + ep = &sc->sc_out_ep[0];
> + for (i=0; i<sc->sc_out_num_endpoints; i++) {
> + rjack = &ep->jacks[0];
> + for (j=0; j<ep->num_jacks; j++) {
> + *rjack = jack;
> + jack->endpoint = ep;
> + jack++;
> + rjack++;
> + }
> + ep++;
>   }
> - ep++;
>   }
> - jack = &sc->sc_in_jacks[0];
> - ep = &sc->sc_in_ep[0];
> - for (i=0; i<sc->sc_in_num_endpoints; i++) {
> - rjack = &ep->jacks[0];
> - for (j=0; j<ep->num_jacks; j++) {
> - *rjack = jack;
> - jack->endpoint = ep;
> - jack++;
> - rjack++;
> + if (sc->sc_in_jacks && sc->sc_in_ep) {
> + jack = &sc->sc_in_jacks[0];
> + ep = &sc->sc_in_ep[0];
> + for (i=0; i<sc->sc_in_num_endpoints; i++) {
> + rjack = &ep->jacks[0];
> + for (j=0; j<ep->num_jacks; j++) {
> + *rjack = jack;
> + jack->endpoint = ep;
> + jack++;
> + rjack++;
> + }
> + ep++;
>   }
> - ep++;
>   }
>  
>   return USBD_NORMAL_COMPLETION;
>