| Message ID | 00eea53890802b679c138fc7f68a0f162261d95c.1517268668.git.gustavo@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gustavo, On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote: > Cast len to const u64 in order to avoid a potential integer > overflow. This variable is being used in a context that expects > an expression of type const u64. > > Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/media/platform/vivid/vivid-cec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c > index b55d278..30240ab 100644 > --- a/drivers/media/platform/vivid/vivid-cec.c > +++ b/drivers/media/platform/vivid/vivid-cec.c > @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts, > if (adap == NULL) > return; > ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + > - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); > + (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL)); This makes no sense. Certainly the const part is pointless. And given that len is always <= 16 there definitely is no overflow. I don't really want this cast in the code. Sorry, Hans > cec_queue_pin_cec_event(adap, false, ts); > ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW); > cec_queue_pin_cec_event(adap, true, ts); >
Hi Hans, Quoting Hans Verkuil <hverkuil@xs4all.nl>: > Hi Gustavo, > > On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote: >> Cast len to const u64 in order to avoid a potential integer >> overflow. This variable is being used in a context that expects >> an expression of type const u64. >> >> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >> --- >> drivers/media/platform/vivid/vivid-cec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/vivid/vivid-cec.c >> b/drivers/media/platform/vivid/vivid-cec.c >> index b55d278..30240ab 100644 >> --- a/drivers/media/platform/vivid/vivid-cec.c >> +++ b/drivers/media/platform/vivid/vivid-cec.c >> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct >> cec_adapter *adap, ktime_t ts, >> if (adap == NULL) >> return; >> ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + >> - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >> + (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL)); > > This makes no sense. Certainly the const part is pointless. And given that > len is always <= 16 there definitely is no overflow. > Yeah, I understand your point and I know there is no chance of an overflow in this particular case. > I don't really want this cast in the code. > > Sorry, > I'm working through all the Linux kernel Coverity reports, and I thought of sending a patch for this because IMHO it doesn't hurt to give the compiler complete information about the arithmetic in which an expression is intended to be evaluated. I agree that the _const_ part is a bit odd. What do you think about the cast to u64 alone? I appreciate your feedback. Thanks -- Gustavo
On 01/30/2018 09:51 AM, Gustavo A. R. Silva wrote: > Hi Hans, > > Quoting Hans Verkuil <hverkuil@xs4all.nl>: > >> Hi Gustavo, >> >> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote: >>> Cast len to const u64 in order to avoid a potential integer >>> overflow. This variable is being used in a context that expects >>> an expression of type const u64. >>> >>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") >>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >>> --- >>> drivers/media/platform/vivid/vivid-cec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/vivid/vivid-cec.c >>> b/drivers/media/platform/vivid/vivid-cec.c >>> index b55d278..30240ab 100644 >>> --- a/drivers/media/platform/vivid/vivid-cec.c >>> +++ b/drivers/media/platform/vivid/vivid-cec.c >>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct >>> cec_adapter *adap, ktime_t ts, >>> if (adap == NULL) >>> return; >>> ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + >>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >>> + (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >> >> This makes no sense. Certainly the const part is pointless. And given that >> len is always <= 16 there definitely is no overflow. >> > > Yeah, I understand your point and I know there is no chance of an > overflow in this particular case. > >> I don't really want this cast in the code. >> >> Sorry, >> > > I'm working through all the Linux kernel Coverity reports, and I > thought of sending a patch for this because IMHO it doesn't hurt to > give the compiler complete information about the arithmetic in which > an expression is intended to be evaluated. > > I agree that the _const_ part is a bit odd. What do you think about > the cast to u64 alone? What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL + I think that forces everything else in the expression to be evaluated as u64. It definitely needs a comment that this fixes a bogus Coverity report. Regards, Hans > > I appreciate your feedback. > > Thanks > -- > Gustavo > > > > >
Quoting Hans Verkuil <hverkuil@xs4all.nl>: > On 01/30/2018 09:51 AM, Gustavo A. R. Silva wrote: >> Hi Hans, >> >> Quoting Hans Verkuil <hverkuil@xs4all.nl>: >> >>> Hi Gustavo, >>> >>> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote: >>>> Cast len to const u64 in order to avoid a potential integer >>>> overflow. This variable is being used in a context that expects >>>> an expression of type const u64. >>>> >>>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") >>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >>>> --- >>>> drivers/media/platform/vivid/vivid-cec.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c >>>> b/drivers/media/platform/vivid/vivid-cec.c >>>> index b55d278..30240ab 100644 >>>> --- a/drivers/media/platform/vivid/vivid-cec.c >>>> +++ b/drivers/media/platform/vivid/vivid-cec.c >>>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct >>>> cec_adapter *adap, ktime_t ts, >>>> if (adap == NULL) >>>> return; >>>> ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + >>>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >>>> + (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >>> >>> This makes no sense. Certainly the const part is pointless. And given that >>> len is always <= 16 there definitely is no overflow. >>> >> >> Yeah, I understand your point and I know there is no chance of an >> overflow in this particular case. >> >>> I don't really want this cast in the code. >>> >>> Sorry, >>> >> >> I'm working through all the Linux kernel Coverity reports, and I >> thought of sending a patch for this because IMHO it doesn't hurt to >> give the compiler complete information about the arithmetic in which >> an expression is intended to be evaluated. >> >> I agree that the _const_ part is a bit odd. What do you think about >> the cast to u64 alone? > > What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL + > > I think that forces everything else in the expression to be evaluated > as u64. > Well, in this case the operator precedence takes place and the expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is computed first. So the issue remains the same. I can switch the expressions as follows: (u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL + CEC_TIM_START_BIT_TOTAL and avoid the cast in the middle. What do you think? > It definitely needs a comment that this fixes a bogus Coverity report. > I actually added the following line to the message changelog: Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") Certainly, I've run across multiple false positives as in this case, but I have also fixed many actual bugs thanks to the Coverity reports. So I think in general it is valuable to take a look into these reports, either if they spot actual bugs or promote code correctness. Thanks -- Gustavo
On 01/30/18 11:55, Gustavo A. R. Silva wrote: > > Quoting Hans Verkuil <hverkuil@xs4all.nl>: > >> On 01/30/2018 09:51 AM, Gustavo A. R. Silva wrote: >>> Hi Hans, >>> >>> Quoting Hans Verkuil <hverkuil@xs4all.nl>: >>> >>>> Hi Gustavo, >>>> >>>> On 01/30/2018 01:33 AM, Gustavo A. R. Silva wrote: >>>>> Cast len to const u64 in order to avoid a potential integer >>>>> overflow. This variable is being used in a context that expects >>>>> an expression of type const u64. >>>>> >>>>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") >>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >>>>> --- >>>>> drivers/media/platform/vivid/vivid-cec.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c >>>>> b/drivers/media/platform/vivid/vivid-cec.c >>>>> index b55d278..30240ab 100644 >>>>> --- a/drivers/media/platform/vivid/vivid-cec.c >>>>> +++ b/drivers/media/platform/vivid/vivid-cec.c >>>>> @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct >>>>> cec_adapter *adap, ktime_t ts, >>>>> if (adap == NULL) >>>>> return; >>>>> ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + >>>>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >>>>> + (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL)); >>>> >>>> This makes no sense. Certainly the const part is pointless. And given that >>>> len is always <= 16 there definitely is no overflow. >>>> >>> >>> Yeah, I understand your point and I know there is no chance of an >>> overflow in this particular case. >>> >>>> I don't really want this cast in the code. >>>> >>>> Sorry, >>>> >>> >>> I'm working through all the Linux kernel Coverity reports, and I >>> thought of sending a patch for this because IMHO it doesn't hurt to >>> give the compiler complete information about the arithmetic in which >>> an expression is intended to be evaluated. >>> >>> I agree that the _const_ part is a bit odd. What do you think about >>> the cast to u64 alone? >> >> What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL + >> >> I think that forces everything else in the expression to be evaluated >> as u64. >> > > Well, in this case the operator precedence takes place and the > expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is computed first. So the > issue remains the same. > > I can switch the expressions as follows: > > (u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL + CEC_TIM_START_BIT_TOTAL What about: 10ULL * len * ... > > and avoid the cast in the middle. > > What do you think? My problem is that (u64)len suggests that there is some problem with len specifically, which isn't true. > >> It definitely needs a comment that this fixes a bogus Coverity report. >> > > I actually added the following line to the message changelog: > Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") That needs to be in the source, otherwise someone will remove the cast (or ULL) at some time in the future since it isn't clear why it is done. And nobody reads commit logs from X years back :-) > > Certainly, I've run across multiple false positives as in this case, > but I have also fixed many actual bugs thanks to the Coverity reports. > So I think in general it is valuable to take a look into these > reports, either if they spot actual bugs or promote code correctness. Regards, Hans
Quoting Hans Verkuil <hverkuil@xs4all.nl>: [...] >>> What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL + >>> >>> I think that forces everything else in the expression to be evaluated >>> as u64. >>> >> >> Well, in this case the operator precedence takes place and the >> expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is computed first. So the >> issue remains the same. >> >> I can switch the expressions as follows: >> >> (u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL + CEC_TIM_START_BIT_TOTAL > > What about: > > 10ULL * len * ... > Yeah, I like it. >> >> and avoid the cast in the middle. >> >> What do you think? > > My problem is that (u64)len suggests that there is some problem with len > specifically, which isn't true. > That's a good point. Actually, I think the same applies for the rest of the patch series. Maybe it is a good idea to send a v2 of the whole patchset with that update. >> >>> It definitely needs a comment that this fixes a bogus Coverity report. >>> >> >> I actually added the following line to the message changelog: >> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") > > That needs to be in the source, otherwise someone will remove the > cast (or ULL) at some time in the future since it isn't clear why > it is done. And nobody reads commit logs from X years back :-) > You're right. I thought you were talking about the changelog. And unless you think otherwise, I think there is no need for any additional code comment if the update you suggest is applied: len * 10ULL * CEC_TIM_DATA_BIT_TOTAL Thanks -- Gustavo
On 01/30/18 12:43, Gustavo A. R. Silva wrote: > > Quoting Hans Verkuil <hverkuil@xs4all.nl>: > > [...] > >>>> What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL + >>>> >>>> I think that forces everything else in the expression to be evaluated >>>> as u64. >>>> >>> >>> Well, in this case the operator precedence takes place and the >>> expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is computed first. So the >>> issue remains the same. >>> >>> I can switch the expressions as follows: >>> >>> (u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL + CEC_TIM_START_BIT_TOTAL >> >> What about: >> >> 10ULL * len * ... >> > > Yeah, I like it. > >>> >>> and avoid the cast in the middle. >>> >>> What do you think? >> >> My problem is that (u64)len suggests that there is some problem with len >> specifically, which isn't true. >> > > That's a good point. Actually, I think the same applies for the rest > of the patch series. Maybe it is a good idea to send a v2 of the whole > patchset with that update. > >>> >>>> It definitely needs a comment that this fixes a bogus Coverity report. >>>> >>> >>> I actually added the following line to the message changelog: >>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") >> >> That needs to be in the source, otherwise someone will remove the >> cast (or ULL) at some time in the future since it isn't clear why >> it is done. And nobody reads commit logs from X years back :-) >> > > You're right. I thought you were talking about the changelog. > > And unless you think otherwise, I think there is no need for any > additional code comment if the update you suggest is applied: > > len * 10ULL * CEC_TIM_DATA_BIT_TOTAL I still think I'd like to see a comment. It is still not obvious why you would want to use ULL here. Please use "10ULL * len", it's actually a bit better to have it in that order (it's 10 bits per character, so '10 * len' is a more logical order). Regards, Hans
Quoting Hans Verkuil <hverkuil@xs4all.nl>: > On 01/30/18 12:43, Gustavo A. R. Silva wrote: >> >> Quoting Hans Verkuil <hverkuil@xs4all.nl>: >> >> [...] >> >>>>> What happens if you do: ((u64)CEC_TIM_START_BIT_TOTAL + >>>>> >>>>> I think that forces everything else in the expression to be evaluated >>>>> as u64. >>>>> >>>> >>>> Well, in this case the operator precedence takes place and the >>>> expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is computed first. So the >>>> issue remains the same. >>>> >>>> I can switch the expressions as follows: >>>> >>>> (u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL + CEC_TIM_START_BIT_TOTAL >>> >>> What about: >>> >>> 10ULL * len * ... >>> >> >> Yeah, I like it. >> >>>> >>>> and avoid the cast in the middle. >>>> >>>> What do you think? >>> >>> My problem is that (u64)len suggests that there is some problem with len >>> specifically, which isn't true. >>> >> >> That's a good point. Actually, I think the same applies for the rest >> of the patch series. Maybe it is a good idea to send a v2 of the whole >> patchset with that update. >> >>>> >>>>> It definitely needs a comment that this fixes a bogus Coverity report. >>>>> >>>> >>>> I actually added the following line to the message changelog: >>>> Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") >>> >>> That needs to be in the source, otherwise someone will remove the >>> cast (or ULL) at some time in the future since it isn't clear why >>> it is done. And nobody reads commit logs from X years back :-) >>> >> >> You're right. I thought you were talking about the changelog. >> >> And unless you think otherwise, I think there is no need for any >> additional code comment if the update you suggest is applied: >> >> len * 10ULL * CEC_TIM_DATA_BIT_TOTAL > > I still think I'd like to see a comment. It is still not obvious why > you would want to use ULL here. > OK. That's fine. > Please use "10ULL * len", it's actually a bit better to have it in > that order (it's 10 bits per character, so '10 * len' is a more logical > order). > OK. I got it. Thanks for all the feedback, Hans. -- Gustavo
diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c index b55d278..30240ab 100644 --- a/drivers/media/platform/vivid/vivid-cec.c +++ b/drivers/media/platform/vivid/vivid-cec.c @@ -83,7 +83,7 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts, if (adap == NULL) return; ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL + - len * 10 * CEC_TIM_DATA_BIT_TOTAL)); + (const u64)len * 10 * CEC_TIM_DATA_BIT_TOTAL)); cec_queue_pin_cec_event(adap, false, ts); ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW); cec_queue_pin_cec_event(adap, true, ts);
Cast len to const u64 in order to avoid a potential integer overflow. This variable is being used in a context that expects an expression of type const u64. Addresses-Coverity-ID: 1454996 ("Unintentional integer overflow") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/media/platform/vivid/vivid-cec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)