Messages in this thread | | | Date | Tue, 30 Jan 2018 05:57:04 -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>:
> 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
| |