Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1644845 > unrolled thread
| Started by | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| First post | 2017-05-18 21:10 +0200 |
| Last post | 2017-05-18 22:40 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[media-dvb-usb-v2] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-18 21:10 +0200
Re: [media-dvb-usb-v2] question about value overwrite Malcolm Priestley <tvboxspy@gmail.com> - 2017-05-18 22:00 +0200
Re: [media-dvb-usb-v2] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-18 22:40 +0200
| From | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| Date | 2017-05-18 21:10 +0200 |
| Subject | [media-dvb-usb-v2] question about value overwrite |
| Message-ID | <tIyYa-6Wp-17@gated-at.bofh.it> |
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
--- 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;
}
What do you think?
I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva
[toc] | [next] | [standalone]
| From | Malcolm Priestley <tvboxspy@gmail.com> |
|---|---|
| Date | 2017-05-18 22:00 +0200 |
| Message-ID | <tIzKy-7VB-11@gated-at.bofh.it> |
| In reply to | #1644845 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Gustavo A. R. Silva" <garsilva@embeddedor.com> |
|---|---|
| Date | 2017-05-18 22:40 +0200 |
| Message-ID | <tIAng-8so-11@gated-at.bofh.it> |
| In reply to | #1644880 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web