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;
-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
enlightenment-cvs mailing list
enlightenment-cvs@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-cvs
