sylikc / jpegview

Fork of JPEGView by David Kleiner - fast and highly configurable viewer/editor for JPEG, BMP, PNG, WEBP, TGA, GIF and TIFF images with a minimal GUI. Basic on-the-fly image processing is provided - allowing adjusting typical parameters as sharpness, color balance, rotation, perspective, contrast and local under-/overexposure.
Other
2.22k stars 128 forks source link

Fix possible crash on displaying very large images #307

Closed KrokusPokus closed 5 months ago

KrokusPokus commented 6 months ago

nFirstY, nSrcLineWidthPadded, nFirstX and nChannels are type int; multiplying them, the result will internally be treated as int values again. For very large images, this can cause integer overflows, with the resulting negative values causing a wrong address in the pSrc pointer, leading to a crash. One possible fix is to cast those values to int64 (long long) before multiplication, as in the current PR does. (Strictly speaking, this fix is not neccessary for nFirstX*nChannels, since that ones' product can't grow that large, with nChannels being a single digit value...)

Alternatively, we could get away with just using uint32 instead of long long, since none of the factors should ever be negative. It would allow for images up to 65535x65535 in size not triggering on overflow, which happens to be our size limit in MaxImageDef.h:

const unsigned int MAX_IMAGE_DIMENSION = 65535;

Btw, it might make sense to also change:

const unsigned int MAX_IMAGE_PIXELS = 1024 * 1024 * 500;

to

const unsigned int MAX_IMAGE_PIXELS = 65535 * 65535;
sylikc commented 5 months ago

I'm not really following the code update. wouldn't a long long + unit8 overflow anyways if the long long value was huge?

KrokusPokus commented 5 months ago
const uint8* pSrc = (uint8*)pDIB + nFirstY*nSrcLineWidthPadded + nFirstX*nChannels;

pSrc is a Ptr-sized address (e.g. has a size of 4 bytes in a 32-bit binary, and 8 bytes in a 64-bit binary) to an uint8-sized value. The uint8-sized value the address actually points to is not important at this point - we are just trying to calculate addresses. In the line above, we are calculating addresses by trying to add an offset of nFirstY*nSrcLineWidthPadded to the initial address in pDIB. Basically, we calculate Ptr = Ptr + Int*Int + Int*Int This fails for very large values in nFirstY and nSrcLineWidthPadded, since both are of type Int, which are signed 32-bit values, and the product will internally be treated as Int as well, causing the resulting Int value to overflow after 2,147,483,647 to -2,147,483,648. So instead instead of adding a (positive) offset to the address in pDIB, we are substracting. By doing so, we will then try to access memory below pDIB (below the start address of our DIB memory area). ...Which causes a crash.

sylikc commented 5 months ago

ok, i also updated MAX_IMAGE_PIXELS, let's hope it fixes the huge file crashes