Re: [PATCH 8/8] platform: vivid-cec: fix potential integer overflow in vivid_cec_pin_adap_events
From: Gustavo A. R. Silva
Date: Tue Jan 30 2018 - 07:19:55 EST
Quoting Hans Verkuil <hverkuil@xxxxxxxxx>:
On 01/30/18 12:43, Gustavo A. R. Silva wrote:
Quoting Hans Verkuil <hverkuil@xxxxxxxxx>:
[...]
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