GNOME Bugzilla – Bug 603998
PCX: Calculating amount of memory to allocate may overflow.
Last modified: 2009-12-09 15:40:46 UTC
When calculating how much memory it should allocate for certain operations, the PCX plugin does it like this: [...] dest = g_new (guchar, width * height); [...] dest = g_new (guchar, width * height * 3); [...] Since it's not explicitly cast, the calculation is done with the type of the "width" variable which may overflow the calculation (since both width and height are signed 32bit ints and hold _unsigned_ 16bit int values). Casting width to gsize causes the whole calculation to be done in that type, avoiding an overflowed result.
Created attachment 149269 [details] [review] Proposed fix: cast allocation calculations
Well, this is a aesthetic problem only, I think. To my knowledge the processor does not make any difference between "signed" and "unsigned" add/mul-calculations, so all it knows about are 32bit integers here. Since g_new boils down to g_malloc, which uses a gsize as its argument the 32 bits in the argument get interpreted as unsigned and we don't really have a problem here. Ok, the *3 of course, but that is dealt with in the fix for bug #603995. Not sure if we really should apply this patch.
Agreed, but only partly: only "width * height * 3" is a real problem, because g_new(...) does the calculation in the type of width (gint32) which may overflow if width and height are near enough to G_MAXUINT16: G_MAXUINT32 < G_MAXUINT16 * G_MAXUINT16 * 3 2^32 - 1 < (2^16 - 1) * (2^16 - 1) * 3 4294967295 < 12884508675 To avoid this overflow, the calculation has to be made with the gsize type in this case. Technically, we could leave it out for the other allocations, but for the sake of consistency (and setting a good example of defensive programming ;-), I'd leave them in.
Review of attachment 149269 [details] [review]: generally looks OK. coding-style-wise: please put spaces between cast and variable. Also add an extra set of parentheses in the last part of the patch, so that the scope of the cast is visible in the code. Please change accordingly and commit.
Review of attachment 149269 [details] [review]: committed as a9671395f657