[poppler] Which check is better?

[Jeff Muizelaar]

On Sun, Feb 05, 2006 at 08:37:24PM +0100, Albert Astals Cid wrote:
> I've been having a look to the final merges kpdf -> poppler and this is one of 
> the few easy things left, which of these merges seem better to you?
> 
> JPXStream.cc about line 824
> 
>       img.nXTiles = (img.xSize - img.xTileOffset + img.xTileSize - 1)
> 	            / img.xTileSize;
>       img.nYTiles = (img.ySize - img.yTileOffset + img.yTileSize - 1)
> 	            / img.yTileSize;
>       nTiles = img.nXTiles * img.nYTiles;
>       // check for overflow before allocating memory
>       if (nTiles == 0 || nTiles / img.nXTiles != img.nYTiles) {
> 	error(getPos(), "Bad tile count in JPX SIZ marker segment");
> 	return gFalse;
>       }
> 
> 
>       img.nXTiles = (img.xSize - img.xTileOffset + img.xTileSize - 1)
> 	            / img.xTileSize;
>       img.nYTiles = (img.ySize - img.yTileOffset + img.yTileSize - 1)
> 	            / img.yTileSize;
>       nTiles = img.nXTiles * img.nYTiles;
>       // check for overflow before allocating memory
>       if (img.nXTiles <= 0 || img.nYTiles <= 0 || img.nXTiles >= INT_MAX / 
> img.nYTiles) {
> 	error(getPos(), "Bad tile count in JPX SIZ marker segment");
> 	return gFalse;
>       }
> 
> 
> ONLY the if is different 
> 
> Any idea of which may be better?

I think the top one is better. The second tests img.nXTiles and
img.nYTiles for < 0 which is never true because they are unsigned.
This makes the code look like it is testing signed ints which is a lie.
In addition the second one is wrong sometimes. e.g.

img.nXTimes = (INT_MAX / 2);
img.nYTimes = 2;

img.nXTimes * img.nYTimes is 0x7ffffffe which fits comfortably in an
unsigned int. In fact it even fits in a signed int.

Of course you probably don't want 0x7ffffffe tiles, but that is for
malloc to decide.

-Jeff