From: <enl...@li...> - 2006-03-22 02:08:49
|
Enlightenment CVS committal Author : raster Project : e17 Module : libs/imlib2 Dir : e17/libs/imlib2/src/modules/loaders Modified Files: loader_bmp.c Log Message: bmp fixes =================================================================== RCS file: /cvsroot/enlightenment/e17/libs/imlib2/src/modules/loaders/loader_bmp.c,v retrieving revision 1.5 retrieving revision 1.6 diff -u -3 -r1.5 -r1.6 --- loader_bmp.c 19 Mar 2006 08:46:16 -0000 1.5 +++ loader_bmp.c 22 Mar 2006 02:08:39 -0000 1.6 @@ -3,6 +3,12 @@ * imlib's old BMP loader */ +/* + * 21.3.2006 - Changes made by Petr Kobalicek + * - Simplify and make secure RLE encoding + * - Fix 16 and 32 bit depth (old code was incorrect and it's commented) + */ + #ifdef HAVE_CONFIG_H # include <config.h> #endif @@ -31,6 +37,13 @@ #define BI_RLE4 2 #define BI_BITFIELDS 3 +/* 21.3.3006 - Use enumeration for RLE encoding. This makes it more readable */ +enum { + RLE_NEXT = 0, /* Next line */ + RLE_END = 1, /* End of RLE encoding */ + RLE_MOVE = 2 /* Move by X and Y (Offset is stored in two next bytes) */ +}; + static int ReadleShort(FILE * file, unsigned short *ret) { @@ -122,6 +135,12 @@ unsigned long rmask = 0xff, gmask = 0xff, bmask = 0xff; unsigned long rshift = 0, gshift = 0, bshift = 0; + /* + * 21.3.2006: + * Added these two variables for RLE. + */ + unsigned char byte1, byte2; + if (im->data) return 0; f = fopen(im->real_file, "rb"); @@ -186,12 +205,12 @@ fclose(f); return 0; } - + if ((w < 1) || (h < 1) || (w > 8192) || (h > 8192)) - { - fclose(f); - return 0; - } + { + fclose(f); + return 0; + } if (bitcount < 16) { @@ -221,12 +240,9 @@ ReadleLong(f, &rmask); for (bit = bitcount - 1; bit >= 0; bit--) { - if (bmask & (1 << bit)) - bshift = bit; - if (gmask & (1 << bit)) - gshift = bit; - if (rmask & (1 << bit)) - rshift = bit; + if (bmask & (1 << bit)) bshift = bit; + if (gmask & (1 << bit)) gshift = bit; + if (rmask & (1 << bit)) rshift = bit; } } else if (bitcount == 16) @@ -326,102 +342,116 @@ } } } + + /* + * 21.3.2006 + * Bug fixes and optimization: + * + * RLE encoding is dangerous and can be used by attackers by creating special files. + * We has 'buffer_ptr' and 'buffer_end' variables and buffer_end points to first + * unaccessible byte in buffer. + * - If we use 'byte = *(buffer_ptr++) in main loop we must check if + * 'buffer_ptr != buffer_end', because special or incomplete bmp file can generate + * segfault (I was writing it, because in RLE we need to read depending count of + * bytes that depends on requester operation). + * SOLUTION: Don't read one byte, read two bytes and check. + * - If RLE teels us than single color length will be larger than allowed, we can + * stop, because bitmap is corrupted or crawled. + * SOLUTION: Check for length ('l' variable in RLE) and break loop if it's invalid + * IMPROVEMENTS: We can stop checking if 'x' is out of rangle, because it never be. + * - In RLE4 use one bigger loop that fills two pixels. This is faster and cleaner. + * If one pixel remains (the tail), do it on end of the loop. + * - If we will check x and y (new line and skipping), we can't go outsize imlib + * image buffer. + */ + if (bitcount == 4) { if (comp == BI_RLE4) { + /* + * 21.3.2006: This is better than using 'if buffer_ptr + 1 < buffer_end' + */ + unsigned char *buffer_end_minus_1 = buffer_end - 1; x = 0; y = 0; - for (i = 0, g = 1; - i < imgsize && g && buffer_ptr < buffer_end; i++) + for (i = 0; i < imgsize && buffer_ptr < buffer_end_minus_1; i++) { - byte = *(buffer_ptr++); - if (byte) - { - unsigned char t1, t2; - - l = byte; - byte = *(buffer_ptr++); - t1 = byte & 0xF; - t2 = (byte >> 4) & 0xF; - for (j = 0; j < l; j++) - { - k = (j & 1) ? t1 : t2; - - if (x >= w) - break; - - *ptr++ = 0xff000000 | - (rgbQuads[k].rgbRed << 16) | - (rgbQuads[k].rgbGreen << 8) | - rgbQuads[k].rgbBlue; - x++; - if (ptr > data_end) - ptr = data_end; - } + byte1 = buffer_ptr[0]; + byte2 = buffer_ptr[1]; + buffer_ptr += 2; + if (byte1) + { + DATA32 t1, t2; + + l = byte1; + /* Check for invalid length */ + if (l + x > w) goto _bail; + + t1 = 0xff000000 | (rgbQuads[byte2 >> 4].rgbRed << 16) | + (rgbQuads[byte2 >> 4].rgbGreen << 8) | + (rgbQuads[byte2 >> 4].rgbBlue ) ; + t2 = 0xff000000 | (rgbQuads[byte2 & 0xF].rgbRed << 16) | + (rgbQuads[byte2 & 0xF].rgbGreen << 8) | + (rgbQuads[byte2 & 0xF].rgbBlue ) ; + for (j = l/2; j; j--) { + ptr[0] = t1; + ptr[1] = t2; + ptr += 2; + } + /* tail */ + if (l & 1) *ptr++ = t1; + x += l; } else { - byte = *(buffer_ptr++); - switch (byte) + switch (byte2) { - case 0: + case RLE_NEXT: x = 0; - y++; - ptr = im->data + ((h - y - 1) - * w * sizeof(DATA32)); - if (ptr > data_end) - ptr = data_end; - break; - case 1: - g = 0; + if (++y >= h) goto _bail; + ptr = im->data + (h - y - 1) * w; break; - case 2: - x += *(buffer_ptr++); - y += *(buffer_ptr++); - ptr = im->data + ((h - y - 1) * w * - sizeof(DATA32)) + x; - if (ptr > data_end) - ptr = data_end; + case RLE_END: + goto _bail; + case RLE_MOVE: + /* Need to read two bytes */ + if (buffer_ptr >= buffer_end_minus_1) goto _bail; + x += buffer_ptr[0]; + y += buffer_ptr[1]; + buffer_ptr += 2; + /* Check for correct coordinates */ + if (x >= w) goto _bail; + if (y >= h) goto _bail; + ptr = im->data + (h - y - 1) * w + x; break; default: - l = byte; - for (j = 0; j < l; j++) - { - char t1 = '\0', t2 = - '\0'; - - if ((j & 1) == 0) - { - byte = *(buffer_ptr++); - t1 = byte & 0xF; - t2 = (byte >> 4) & 0xF; - } - k = (j & 1) ? t1 : t2; - - if (x >= w) - { - buffer_ptr += (l - j) / 2; - break; - } - - *ptr++ = 0xff000000 | - (rgbQuads[k].rgbRed << 16) | - (rgbQuads[k].rgbGreen << 8) | - rgbQuads[k].rgbBlue; - x++; - - if (ptr > data_end) - ptr = data_end; - - } + l = byte2; + /* Check for invalid length and valid buffer size */ + if (l + x > w) goto _bail; + if (buffer_ptr + (l >> 1) + (l & 1) > buffer_end) goto _bail; + + for (j = l/2; j; j--) { + byte = *buffer_ptr++; + ptr[0] = 0xff000000 | (rgbQuads[byte >> 4].rgbRed << 16) | + (rgbQuads[byte >> 4].rgbGreen << 8) | + (rgbQuads[byte >> 4].rgbBlue ) ; + ptr[1] = 0xff000000 | (rgbQuads[byte & 0xF].rgbRed << 16) | + (rgbQuads[byte & 0xF].rgbGreen << 8) | + (rgbQuads[byte & 0xF].rgbBlue ) ; + ptr += 2; + } + if (l & 1) { + byte = *buffer_ptr++; + *ptr++ = 0xff000000 | (rgbQuads[byte >> 4].rgbRed << 16) | + (rgbQuads[byte >> 4].rgbGreen << 8) | + (rgbQuads[byte >> 4].rgbBlue ) ; + } + x += l; if ((l & 3) == 1) - { - tempchar = *(buffer_ptr++); - tempchar = *(buffer_ptr++); - } + buffer_ptr += 2; else if ((l & 3) == 2) buffer_ptr++; break; @@ -497,78 +527,63 @@ { if (comp == BI_RLE8) { + /* + * 21.3.2006: This is better than using 'if buffer_ptr + 1 < buffer_end' + */ + unsigned char *buffer_end_minus_1 = buffer_end - 1; x = 0; y = 0; - for (i = 0, g = 1; - i < imgsize && buffer_ptr < buffer_end && g; i++) + for (i = 0; i < imgsize && buffer_ptr < buffer_end_minus_1 && g; i++) { - byte = *(buffer_ptr++); - if (byte) - { - l = byte; - byte = *(buffer_ptr++); - for (j = 0; j < l; j++) - { - if (x >= w) - break; - - *ptr++ = 0xff000000 | - (rgbQuads[byte].rgbRed << 16) | - (rgbQuads[byte].rgbGreen << 8) | - rgbQuads[byte].rgbBlue; - - x++; - if (ptr > data_end) - ptr = data_end; - } + byte1 = buffer_ptr[0]; + byte2 = buffer_ptr[1]; + buffer_ptr += 2; + if (byte1) + { + DATA32 pix = 0xff000000 | (rgbQuads[byte2].rgbRed << 16) | + (rgbQuads[byte2].rgbGreen << 8) | + (rgbQuads[byte2].rgbBlue ) ; + l = byte1; + if (x + l > w) goto _bail; + for (j = l; j; j--) *ptr++ = pix; + x += l; } else { - byte = *(buffer_ptr++); - switch (byte) + switch (byte2) { - case 0: + case RLE_NEXT: x = 0; - y++; - ptr = im->data + ((h - y - 1) - * w * sizeof(DATA32)); - if (ptr > data_end) - ptr = data_end; + if (++y >= h) goto _bail; + ptr = im->data + ((h - y - 1) * w) + x; break; - case 1: - g = 0; - break; - case 2: - x += *(buffer_ptr++); - y += *(buffer_ptr++); - ptr = im->data + ((h - y - 1) - * w * - sizeof(DATA32)) + - (x * sizeof(DATA32)); - if (ptr > data_end) - ptr = data_end; + case RLE_END: + goto _bail; + case RLE_MOVE: + /* Need to read two bytes */ + if (buffer_ptr >= buffer_end_minus_1) goto _bail; + x += buffer_ptr[0]; + y += buffer_ptr[1]; + buffer_ptr += 2; + /* Check for correct coordinates */ + if (x >= w) goto _bail; + if (y >= h) goto _bail; + ptr = im->data + ((h - y - 1) * w) + x; break; default: - l = byte; + l = byte2; + if (x + l > w) goto _bail; + if (buffer_ptr + l > buffer_end) goto _bail; for (j = 0; j < l; j++) { byte = *(buffer_ptr++); - if (x >= w) - { - buffer_ptr += l - j; - break; - } - *ptr++ = 0xff000000 | (rgbQuads[byte].rgbRed << 16) | (rgbQuads[byte].rgbGreen << 8) | rgbQuads[byte].rgbBlue; - x++; - - if (ptr > data_end) - ptr = data_end; } + x += l; if (l & 1) buffer_ptr++; break; @@ -639,17 +654,30 @@ } else if (bitcount == 16) { + /* 21.3.2006 - Need to check for buffer_ptr + 1 < buffer_end */ + unsigned char *buffer_end_minus_1 = buffer_end - 1; skip = (((w * 16 + 31) / 32) * 4) - (w * 2); for (y = 0; y < h; y++) { - for (x = 0; x < w && buffer_ptr < buffer_end; x++) + for (x = 0; x < w && buffer_ptr < buffer_end_minus_1; x++) { - r = ((unsigned short)(*buffer_ptr) & rmask) >> rshift; - g = ((unsigned short)(*buffer_ptr) & gmask) >> gshift; - b = ((unsigned short)(*(buffer_ptr)) & bmask) >> - bshift; - *ptr++ = 0xff000000 | (r << 16) | (g << 8) | b; - buffer_ptr += 2; + /* + * THIS WAS OLD CODE + * + * r = ((unsigned short)(*buffer_ptr) & rmask) >> rshift; + * g = ((unsigned short)(*buffer_ptr) & gmask) >> gshift; + * b = ((unsigned short)(*(buffer_ptr++)) & bmask) >> + * bshift; + * *ptr++ = 0xff000000 | (r << 16) | (g << 8) | b; + */ + /* TODO: I don't know if [rgb]shift are calculated correctly, because we + * 16 bit depth losses some values (bits). + */ + unsigned short pix = *(unsigned short *)buffer_ptr; + *ptr++ = 0xff000000 | (((pix & rmask) >> rshift) << 16) | + (((pix & gmask) >> gshift) << 8) | + (((pix & bmask) >> bshift) ) ; + buffer_ptr += 2; } ptr -= w * 2; buffer_ptr += skip; @@ -678,10 +706,12 @@ } else if (bitcount == 24) { + /* 21.3.2006 - Fix: need to check for buffer_ptr + 2 < buffer_end */ + unsigned char *buffer_end_minus_2 = buffer_end - 2; skip = (4 - ((w * 3) % 4)) & 3; for (y = 0; y < h; y++) { - for (x = 0; x < w && buffer_ptr < buffer_end; x++) + for (x = 0; x < w && buffer_ptr < buffer_end_minus_2; x++) { b = *(buffer_ptr++); g = *(buffer_ptr++); @@ -715,16 +745,30 @@ } else if (bitcount == 32) { + /* 21.3.2006 - Need to check buffer_ptr + 3 < buffer_end */ + unsigned char *buffer_end_minus_3 = buffer_end_minus_3; skip = (((w * 32 + 31) / 32) * 4) - (w * 4); for (y = 0; y < h; y++) { - for (x = 0; x < w && buffer_ptr < buffer_end; x++) + for (x = 0; x < w && buffer_ptr < buffer_end_minus_3; x++) { - r = ((unsigned long)(*buffer_ptr) & rmask) >> rshift; - g = ((unsigned long)(*buffer_ptr) & gmask) >> gshift; - b = ((unsigned long)(*buffer_ptr) & bmask) >> bshift; - *ptr++ = 0xff000000 | (r << 16) | (g << 8) | b; - buffer_ptr+=4; + /* + * THIS WAS OLD CODE: I don't understand it and it's invalid. + * + * r = ((unsigned long)(*buffer_ptr) & rmask) >> rshift; + * g = ((unsigned long)(*buffer_ptr) & gmask) >> gshift; + * b = ((unsigned long)(*buffer_ptr) & bmask) >> bshift; + * *ptr++ = 0xff000000 | (r << 16) | (g << 8) | b; + * r = *(buffer_ptr++); + * r = *(buffer_ptr++); + */ + + /* TODO: What about alpha channel...Is used? */ + DATA32 pix = *(unsigned int *)buffer_ptr; + *ptr++ = 0xff000000 | (((pix & rmask) >> rshift) << 16) | + (((pix & gmask) >> gshift) << 8) | + (((pix & bmask) >> bshift) ) ; + buffer_ptr += 4; } ptr -= w * 2; buffer_ptr += skip; @@ -751,6 +795,7 @@ } } } +_bail: free(buffer); } return 1; |