1228314 - [Static Analysis][INT Overflow] Function LookupBestMatch from SurfaceCache.cpp overflow on idealArea

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Andi [:andi]]

The Static Analysis tool Scan-Build added that variable idealArea will be assigned a potentially overflow value from int32.

Comment 1

[Andi [:andi]]

Attachment: Bug 1228314.diff

Hello Seth could you please take a look other this issue and tell me if the patch seems ok?

Comment 2

[Andi [:andi]]

A correction to my first post the error was generated by Coverity not Scan-build

Comment 3

[Andi [:andi]]

Attachment: MozReview Request: Bug 1228314 - added static_cast<int64> in order to avoid overflow. r?seth

Review commit: https://reviewboard.mozilla.org/r/31241/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31241/

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 8709049 [details]
MozReview Request: Bug 1228314 - added static_cast<int64>  in order to avoid overflow. r?seth

Sorry, I am not ideal for this review.

Comment 5

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

Comment on attachment 8709049 [details]
MozReview Request: Bug 1228314 - added static_cast<int64>  in order to avoid overflow. r?seth

https://reviewboard.mozilla.org/r/31241/#review33283

Looks good! Thanks for identifying this issue.

::: image/SurfaceCache.cpp:330
(Diff revision 1)
> -      int64_t idealArea = idealKey.Size().width * idealKey.Size().height;
> +      int64_t idealArea = static_cast<int64_t>(idealKey.Size().width) * static_cast<int64_t>(idealKey.Size().height);

This looks over 80 columns. Please insert a line break right after the '*'.

Comment 6

[Andi [:andi]]

Comment on attachment 8709049 [details]
MozReview Request: Bug 1228314 - added static_cast<int64>  in order to avoid overflow. r?seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31241/diff/1-2/

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9727cdebb2ee

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9727cdebb2ee

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8709049 [details]
MozReview Request: Bug 1228314 - added static_cast<int64>  in order to avoid overflow. r?seth

Need this for bug 1251091.

Approval Request Comment
[Feature/regressing bug #]: bug 1119774 added this code
[User impact if declined]: needed for bug 1251091
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: safe
[String/UUID change made/needed]: none

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 46 affected.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8709049 [details]
MozReview Request: Bug 1228314 - added static_cast<int64>  in order to avoid overflow. r?seth

We need this for the other related fixes for surface issues
Please uplift to aurora.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9563ef17e18