243763 - warning C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P5)

Description

[timeless]

r:/mozilla\modules\libpr0n\decoders\icon\win\nsIconChannel.cpp(307) : warning 
C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data
r:/mozilla\modules\libpr0n\decoders\icon\win\nsIconChannel.cpp(308) : warning 
C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data

Comment 1

[[e-mail]]

Because of the API used we know that the icon can't have dimensions out of
bounds, right? So we just need to shut up the compiler?

Comment 2

[JP Rosevear [:jpr]]

Still an issue, though the code has moved:
e:/builds/moz2_slave/m-cen-w32/build/image/decoders/icon/win/nsIconChannel.cpp(561) : warning C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data

e:/builds/moz2_slave/m-cen-w32/build/image/decoders/icon/win/nsIconChannel.cpp(562) : warning C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data

Comment 3

[filidautore]

Attachment: Make the cast explicit

I only made the cast explicit.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 699195 [details] [diff] [review]
Make the cast explicit

Review of attachment 699195 [details] [diff] [review]:
-----------------------------------------------------------------

While this code looks great, I don't know enough about GetDIBits to comment on whether it's actually not going to overflow - i.e., whether we have a latent bug here.

Comment 5

[[e-mail]]

(In reply to Joe Drew from comment #4)
> While this code looks great, I don't know enough about GetDIBits to comment
> on whether it's actually not going to overflow - i.e., whether we have a
> latent bug here.

Seems legit, the bitmap came from an HICON in the first place.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 699195 [details] [diff] [review]
Make the cast explicit

Review of attachment 699195 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

I think GetIconInfo will always return handles to bitmaps that have the correct dimensions but we might as well add some sanity checks while we're here.

Could you add some extra checks here:
http://dxr.mozilla.org/mozilla-central/image/decoders/icon/win/nsIconChannel.cpp.html#l489

colorHeader.biWidth >= 0 && colorHeader.biWidth <= 255 &&
colorHeader.biHeight >= 0 && colorHeader.biHeight <= 255 &&

Re-request review after that and we should be good to land this patch.

Comment 7

[filidautore]

Attachment: Make the cast explicit, plus feedback

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 700283 [details] [diff] [review]
Make the cast explicit, plus feedback

Review of attachment 700283 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I'll run it through try and if it passes we can land it!
Thanks again for the patch.

Comment 9

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c2be62cb0c0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c2be62cb0c0