Messages in this thread | | | Date | Tue, 30 Jan 2018 05:43:48 -0600 | From | "Gustavo A. R. Silva" <> | Subject | Re: [PATCH 8/8] platform: vivid-cec: fix potential integer overflow in vivid_cec_pin_adap_events |
| |
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
| |