1228704 - [Static Analysis] OVERFLOW_BEFORE_WIDEN defect reported in SurfaceCache.cpp

Status: RESOLVED DUPLICATE of bug 1251742

(Core :: Graphics: ImageLib, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

There is a new defect detected by Coverity Scan, which, I think, is worth a fix, by either making the variables int32_t instead if we don't think it would ever overflow, or casting the first factor to int64_t before applying the multiplication.

Details:
> *** CID 1340239:    (OVERFLOW_BEFORE_WIDEN)
> /image/SurfaceCache.cpp: 331 in mozilla::image::ImageSurfaceCache::LookupBestMatch(const mozilla::image::SurfaceKey &)()
> 325           SurfaceKey bestMatchKey = bestMatch->GetSurfaceKey();
> 326
> 327           // Compare sizes. We use an area-based heuristic here instead of computing a
> 328           // truly optimal answer, since it seems very unlikely to make a difference
> 329           // for realistic sizes.
> 330           int64_t idealArea = idealKey.Size().width * idealKey.Size().height;
> >>>     CID 1340239:    (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "aSurfaceKey->Size().width * aSurfaceKey->Size().height" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "int64_t" (64 bits, signed).
> 331           int64_t surfaceArea = aSurfaceKey.Size().width * aSurfaceKey.Size().height;
> 332           int64_t bestMatchArea =
> 333             bestMatchKey.Size().width * bestMatchKey.Size().height;
> 334
> 335           // If the best match is smaller than the ideal size, prefer bigger sizes.
> 336           if (bestMatchArea < idealArea) {
> /image/SurfaceCache.cpp: 333 in mozilla::image::ImageSurfaceCache::LookupBestMatch(const mozilla::image::SurfaceKey &)()
> 327           // Compare sizes. We use an area-based heuristic here instead of computing a
> 328           // truly optimal answer, since it seems very unlikely to make a difference
> 329           // for realistic sizes.
> 330           int64_t idealArea = idealKey.Size().width * idealKey.Size().height;
> 331           int64_t surfaceArea = aSurfaceKey.Size().width * aSurfaceKey.Size().height;
> 332           int64_t bestMatchArea =
> >>>     CID 1340239:    (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "bestMatchKey.Size().width * bestMatchKey.Size().height" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "int64_t" (64 bits, signed).
> 333             bestMatchKey.Size().width * bestMatchKey.Size().height;
> 334
> 335           // If the best match is smaller than the ideal size, prefer bigger sizes.
> 336           if (bestMatchArea < idealArea) {
> 337             if (surfaceArea > bestMatchArea) {
> 338               bestMatch = surface;
> /image/SurfaceCache.cpp: 330 in mozilla::image::ImageSurfaceCache::LookupBestMatch(const mozilla::image::SurfaceKey &)()
> 324
> 325           SurfaceKey bestMatchKey = bestMatch->GetSurfaceKey();
> 326
> 327           // Compare sizes. We use an area-based heuristic here instead of computing a
> 328           // truly optimal answer, since it seems very unlikely to make a difference
> 329           // for realistic sizes.
> >>>     CID 1340239:    (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "idealKey->Size().width * idealKey->Size().height" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "int64_t" (64 bits, signed).
> 330           int64_t idealArea = idealKey.Size().width * idealKey.Size().height;
> 331           int64_t surfaceArea = aSurfaceKey.Size().width * aSurfaceKey.Size().height;
> 332           int64_t bestMatchArea =
> 333             bestMatchKey.Size().width * bestMatchKey.Size().height;
> 334
> 335           // If the best match is smaller than the ideal size, prefer bigger sizes.

This defect is initially introduced in bug 1119774, but the code was moved for refactor recently in bug 1186796, so it is detected again.

Comment 1

[Benoit Girard (:BenWa)]

What should we do this with :seth?

Comment 2

[Seth Fowler [:seth] [:s2h]]

(In reply to Benoit Girard (:BenWa) from comment #1)
> What should we do this with :seth?

Let's go ahead and fix this as part of the upcoming SurfaceCache refactoring.

Comment 3

[Timothy Nikkel (:tnikkel)]

Should be fixed by bug 1251742 and bug 1228314.