[media-dvb-usb-v2] question about value overwrite

Commit Message

Gustavo A. R. Silva May 18, 2017, 7:09 p.m. UTC

Hello everybody,

While looking into Coverity ID 1226934 I ran into the following piece  
of code at drivers/media/usb/dvb-usb-v2/lmedm04.c:205

205static int lme2510_stream_restart(struct dvb_usb_device *d)
206{
207        struct lme2510_state *st = d->priv;
208        u8 all_pids[] = LME_ALL_PIDS;
209        u8 stream_on[] = LME_ST_ON_W;
210        int ret;
211        u8 rbuff[1];
212        if (st->pid_off)
213                ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
214                        rbuff, sizeof(rbuff));
215        /*Restart Stream Command*/
216        ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
217                        rbuff, sizeof(rbuff));
218        return ret;
219}

The issue is that the value store in variable _ret_ at line 213 is  
overwritten by the one stored at line 216, before it can be used.

My question is if an _else_ statement is missing, or the variable  
assignment at line 213 should be removed, leaving just the call  
lme2510_usb_talk(d, all_pids, sizeof(all_pids), rbuff, sizeof(rbuff));  
in place.

Maybe either of the following patches could be applied:

index 924adfd..d573144 100644
What do you think?

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

Comments

Malcolm Priestley May 18, 2017, 7:55 p.m. UTC | #1

Hi

On 18/05/17 20:09, Gustavo A. R. Silva wrote:
> 
> Hello everybody,
> 
> While looking into Coverity ID 1226934 I ran into the following piece of 
> code at drivers/media/usb/dvb-usb-v2/lmedm04.c:205
> 
> 205static int lme2510_stream_restart(struct dvb_usb_device *d)
> 206{
> 207        struct lme2510_state *st = d->priv;
> 208        u8 all_pids[] = LME_ALL_PIDS;
> 209        u8 stream_on[] = LME_ST_ON_W;
> 210        int ret;
> 211        u8 rbuff[1];
> 212        if (st->pid_off)
> 213                ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
> 214                        rbuff, sizeof(rbuff));
> 215        /*Restart Stream Command*/
> 216        ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
> 217                        rbuff, sizeof(rbuff));
> 218        return ret;
> 219}

It is a mistake it should have been ORed ad in |= as lme2510_usb_talk 
only returns three states.

So if an error is in the running it will be returned to user.

The first of your patches is better and more or less the same, the 
second would break driver, restart is not an else condition.

Regards


Malcolm

Patch

--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -207,15 +207,15 @@  static int lme2510_stream_restart(struct  
dvb_usb_device *d)
         struct lme2510_state *st = d->priv;
         u8 all_pids[] = LME_ALL_PIDS;
         u8 stream_on[] = LME_ST_ON_W;
-       int ret;
         u8 rbuff[1];
+
         if (st->pid_off)
-               ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
+               lme2510_usb_talk(d, all_pids, sizeof(all_pids),
                         rbuff, sizeof(rbuff));
+
         /*Restart Stream Command*/
-       ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
+       return lme2510_usb_talk(d, stream_on, sizeof(stream_on),
                         rbuff, sizeof(rbuff));
-       return ret;
  }

index 924adfd..dd51f05 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -207,15 +207,15 @@  static int lme2510_stream_restart(struct  
dvb_usb_device *d)
         struct lme2510_state *st = d->priv;
         u8 all_pids[] = LME_ALL_PIDS;
         u8 stream_on[] = LME_ST_ON_W;
-       int ret;
         u8 rbuff[1];
+
         if (st->pid_off)
-               ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
+               return lme2510_usb_talk(d, all_pids, sizeof(all_pids),
                         rbuff, sizeof(rbuff));
-       /*Restart Stream Command*/
-       ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
+       else
+               /*Restart Stream Command*/
+               return lme2510_usb_talk(d, stream_on, sizeof(stream_on),
                         rbuff, sizeof(rbuff));
-       return ret;
  }