On Mon 28 May 2018 03:49:07 PM CEST, Peter Maydell wrote:
> On 28 May 2018 at 09:58, Alberto Garcia <be...@igalia.com> wrote:
>> On Mon 28 May 2018 10:38:55 AM CEST, Kevin Wolf wrote:
>>>> > + if (!refcount_cache_size_set) {
>>>> > + *refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE *
>>>> > s->cluster_size;
>>>>
>>>> ...but in the else clause down here, we don't have the cast, and
>>>> Coverity complains that we evaluate a 32-bit result and then put it
>>>> in a 64-bit variable. Should this have the (uint64_t) cast as well ?
>>
>> I suppose that's not because we put a 32-bit result in a 64-bit
>> variable, but because it could theoretically overflow (if
>> s->cluster_size > INT32_MAX / 4).
>
> Well, coverity warns because it's one of those things that could be
> correct code, or could be the author tripping over one of C's
> less-than-obvious traps. 32x32->32 multiplies are just as susceptible
> to overflow, but the heuristic Coverity uses is "calculated a 32-bit
> multiply result and put it in a 64-bit variable", on the assumption
> that the type of the destination implies that whatever you're
> calculating could well be that big, and "truncate the result of my
> multiply even though it would fit in the destination" is a bit
> unexpected. Coverity's wrong quite a bit on this one, naturally, but
> it's usually easier to shut it up on the wrong guesses for the benefit
> of the times when it turns out to be right.
Yes, it's a fair warning. I'll send a patch.
Berto
