Messages in this thread Patch in this message | | | Date | Thu, 18 May 2017 15:08:38 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [media-dvb-usb-v2] question about value overwrite |
| |
Hi Malcolm,
Quoting Malcolm Priestley <tvboxspy@gmail.com>:
> 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. >
I see now. The idea is to code something similar to the following piece of code in the same file:
242 243 ret |= lme2510_usb_talk(d, pid_buff , 244 sizeof(pid_buff) , rbuf, sizeof(rbuf)); 245 246 if (st->stream_on) 247 ret |= lme2510_stream_restart(d); 248 249 return ret;
right?
So in this case, the following patch would properly fix the bug:
index 924adfd..3ab1754 100644 --- a/drivers/media/usb/dvb-usb-v2/lmedm04.c +++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c @@ -207,13 +207,14 @@ 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; + int ret = 0; u8 rbuff[1]; + if (st->pid_off) ret = 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), + ret |= lme2510_usb_talk(d, stream_on, sizeof(stream_on), rbuff, sizeof(rbuff)); return ret; } What do you think?
> 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. >
Thank you for the clarification. -- Gustavo A. R. Silva
| |