Last Comment Bug 243763 - warning C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data
: warning C4244: '=' : conversion from 'LONG' to 'PRInt8', possible loss of data
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Windows XP
: P5 minor (vote)
: mozilla21
Assigned To: filidautore
:
: Milan Sreckovic [:milan]
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2004-05-16 16:16 PDT by timeless
Modified: 2013-01-22 03:05 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make the cast explicit (1.36 KB, patch)
2013-01-08 07:05 PST, filidautore
netzen: feedback+
Details | Diff | Splinter Review
Make the cast explicit, plus feedback (2.66 KB, patch)
2013-01-10 03:03 PST, filidautore
netzen: review+
Details | Diff | Splinter Review

Description timeless 2004-05-16 16:16:31 PDT
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 neil@parkwaycc.co.uk 2004-05-16 16:25:29 PDT
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] 2011-12-05 12:44:38 PST
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 2013-01-08 07:05:47 PST
Created attachment 699195 [details] [diff] [review]
Make the cast explicit

I only made the cast explicit.
Comment 4 Joe Drew (not getting mail) 2013-01-08 09:44:01 PST
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 neil@parkwaycc.co.uk 2013-01-08 11:14:15 PST
(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] 2013-01-09 10:41:06 PST
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 2013-01-10 03:03:06 PST
Created attachment 700283 [details] [diff] [review]
Make the cast explicit, plus feedback
Comment 8 Brian R. Bondy [:bbondy] 2013-01-10 11:55:07 PST
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 10 Ryan VanderMeulen [:RyanVM] 2013-01-12 12:51:40 PST
https://hg.mozilla.org/mozilla-central/rev/9c2be62cb0c0

Note You need to log in before you can comment on or make changes to this bug.