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.
thanks
-- PMM
