| Message ID | 1369999573-15449-1-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
Dear Piotr Wilczek, In message <1369999573-15449-1-git-send-email-p.wilczek@samsung.com> you wrote: > When compressed image is loaded, it must be decompressed > to an aligned address + 2 to avoid unaligned access exception > on some ARM platforms. If you do this, you must also account for the up to 2 additional bytes needed in the allocated buffer. Otherwise you might write over the end of the buffer... Best regards, Wolfgang Denk
Hello Piotr, On 05/31/2013 02:26 PM, Piotr Wilczek wrote: > -bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) > +bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp, > + void **alloc_addr) > { > void *dst; > unsigned long len; > @@ -60,7 +65,14 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) > puts("Error: malloc in gunzip failed!\n"); > return NULL; > } > - if (gunzip(dst, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr, &len) != 0) { > + > + bmp = dst; > + > + /* align to 32-bit-aligned-address + 2 */ > + if ((unsigned int)bmp % 0x04 != 0x02) > + bmp = (bmp_image_t *)(((unsigned int)dst + 0x02) & ~0x01); This is wrong. Suppose that bmp % 4 == 3, then (dst + 2) % 4 == 1, and thus ((dst + 2) & ~1) % 4 == 0, which is not an aligned+2 address. > + > + if (gunzip(bmp, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr, &len) != 0) { > free(dst); > return NULL; > } > @@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) > puts("Image could be truncated" > " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n"); >
Dear Wolfgang Denk, > -----Original Message----- > From: Wolfgang Denk [mailto:wd@denx.de] > Sent: Friday, May 31, 2013 4:23 PM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; Minkyu Kang; Kyungmin Park; Lukasz Majewski; > Anatolij Gustschin > Subject: Re: [PATCH] lcd: align bmp header when uncopmressing image > > Dear Piotr Wilczek, > > In message <1369999573-15449-1-git-send-email-p.wilczek@samsung.com> > you wrote: > > When compressed image is loaded, it must be decompressed to an > aligned > > address + 2 to avoid unaligned access exception on some ARM > platforms. > > If you do this, you must also account for the up to 2 additional bytes > needed in the allocated buffer. > > Otherwise you might write over the end of the buffer... > Because 8-byte alignment is guaranteed by malloc I don't think might over write anything when moving by only 2 bytes. But you are right that in principle extra bytes should be allocated. > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > Die ganzen Zahlen hat der liebe Gott geschaffen, alles andere ist > Menschenwerk... Leopold Kronecker Best regards, Piotr Wilczek
Hello Nikita, > -----Original Message----- > From: Nikita Kiryanov [mailto:nikita@compulab.co.il] > Sent: Sunday, June 02, 2013 1:06 PM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; Kyungmin Park > Subject: Re: [U-Boot] [PATCH] lcd: align bmp header when uncopmressing > image > > Hello Piotr, > > On 05/31/2013 02:26 PM, Piotr Wilczek wrote: > > -bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) > > +bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp, > > + void **alloc_addr) > > { > > void *dst; > > unsigned long len; > > @@ -60,7 +65,14 @@ bmp_image_t *gunzip_bmp(unsigned long addr, > unsigned long *lenp) > > puts("Error: malloc in gunzip failed!\n"); > > return NULL; > > } > > - if (gunzip(dst, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr, > &len) != 0) { > > + > > + bmp = dst; > > + > > + /* align to 32-bit-aligned-address + 2 */ > > + if ((unsigned int)bmp % 0x04 != 0x02) > > + bmp = (bmp_image_t *)(((unsigned int)dst + 0x02) & ~0x01); > > This is wrong. Suppose that bmp % 4 == 3, then (dst + 2) % 4 == 1, and > thus ((dst + 2) & ~1) % 4 == 0, which is not an aligned+2 address. > You are right but here I'm aligning a pointer returned by malloc which is guaranteed to be aligned to 8 bytes. In fact it is sufficient only to add two bytes. Anyway, to make the code universal I provide a better alignment method. > > + > > + if (gunzip(bmp, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr, > &len) > > +!= 0) { > > free(dst); > > return NULL; > > } > > @@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, > unsigned long *lenp) > > puts("Image could be truncated" > > " (increase > CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n"); > > > > -- > Regards, > Nikita. Best regards, Piotr
Dear Piotr Wilczek, In message <000601ce6021$fcb8f490$f62addb0$%wilczek@samsung.com> you wrote: > > > If you do this, you must also account for the up to 2 additional bytes > > needed in the allocated buffer. > > > > Otherwise you might write over the end of the buffer... > > > Because 8-byte alignment is guaranteed by malloc I don't think might > over write anything when moving by only 2 bytes. Oops??? Initial alignment has NOTHING to do with writing over the allocated end of memory! > But you are right that in principle extra bytes should be allocated. This is not a questions of "pricniple", but plainly of invoking undefined behaviour. Please fix. Best regards, Wolfgang Denk
Dear Piotr Wilczek, In message <000701ce6025$173886c0$45a99440$%wilczek@samsung.com> you wrote: > > > > + /* align to 32-bit-aligned-address + 2 */ > > > + if ((unsigned int)bmp % 0x04 != 0x02) > > > + bmp = (bmp_image_t *)(((unsigned int)dst + 0x02) & ~0x01); > > > > This is wrong. Suppose that bmp % 4 == 3, then (dst + 2) % 4 == 1, and > > thus ((dst + 2) & ~1) % 4 == 0, which is not an aligned+2 address. > > > You are right but here I'm aligning a pointer returned by malloc which is > guaranteed to be aligned to 8 bytes. In fact it is sufficient only to add > two bytes. Such assumptions are black magic at best. Please always consider the mentalk welfare of your follow programmers who for example might change this into a character array (say, a buffer on the stack) and suddenly experience mysetrious crashes. Please always write code such that it is NOT based on non-obvious requirements. > Anyway, to make the code universal I provide a better alignment method. Thanks. Best regards, Wolfgang Denk
Dear Wolfgang Denk, > -----Original Message----- > From: Wolfgang Denk [mailto:wd@denx.de] > Sent: Monday, June 03, 2013 1:16 PM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; 'Minkyu Kang'; 'Kyungmin Park'; Lukasz > Majewski; 'Anatolij Gustschin' > Subject: Re: [PATCH] lcd: align bmp header when uncopmressing image > > Dear Piotr Wilczek, > > In message <000601ce6021$fcb8f490$f62addb0$%wilczek@samsung.com> you > wrote: > > > > > If you do this, you must also account for the up to 2 additional > > > bytes needed in the allocated buffer. > > > > > > Otherwise you might write over the end of the buffer... > > > > > Because 8-byte alignment is guaranteed by malloc I don't think might > > over write anything when moving by only 2 bytes. > > Oops??? Initial alignment has NOTHING to do with writing over the > allocated end of memory! > No, I meant only that malloc allocates memory in at least 4-byte resolution. I surely should have allocated extra memory for the aligned header in the first patch. Please see my fixed patch. Best regards, Piotr Wilczek
Dear Piotr Wilczek, In message <000b01ce6064$b52a7de0$1f7f79a0$%wilczek@samsung.com> you wrote: > > > Oops??? Initial alignment has NOTHING to do with writing over the > > allocated end of memory! > > No, I meant only that malloc allocates memory in at least 4-byte resolution. Does it? I see no such guarantee in the specification. Best regards, Wolfgang Denk
diff --git a/common/cmd_bmp.c b/common/cmd_bmp.c index 5a52edd..fea4708 100644 --- a/common/cmd_bmp.c +++ b/common/cmd_bmp.c @@ -38,14 +38,19 @@ static int bmp_info (ulong addr); /* * Allocate and decompress a BMP image using gunzip(). * - * Returns a pointer to the decompressed image data. Must be freed by - * the caller after use. + * Returns a pointer to the decompressed image data. This pointer is + * is aligned to 32-bit-aligned-address + 2. + * See doc/README.displaying-bmps for explanation. + * + * The allocation address is passed to 'alloc_addr' and must be freed + * by the caller after use. * * Returns NULL if decompression failed, or if the decompressed data * didn't contain a valid BMP signature. */ #ifdef CONFIG_VIDEO_BMP_GZIP -bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) +bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp, + void **alloc_addr) { void *dst; unsigned long len; @@ -60,7 +65,14 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) puts("Error: malloc in gunzip failed!\n"); return NULL; } - if (gunzip(dst, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr, &len) != 0) { + + bmp = dst; + + /* align to 32-bit-aligned-address + 2 */ + if ((unsigned int)bmp % 0x04 != 0x02) + bmp = (bmp_image_t *)(((unsigned int)dst + 0x02) & ~0x01); + + if (gunzip(bmp, CONFIG_SYS_VIDEO_LOGO_MAX_SIZE, (uchar *)addr, &len) != 0) { free(dst); return NULL; } @@ -68,8 +80,6 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) puts("Image could be truncated" " (increase CONFIG_SYS_VIDEO_LOGO_MAX_SIZE)!\n"); - bmp = dst; - /* * Check for bmp mark 'BM' */ @@ -81,10 +91,12 @@ bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) debug("Gzipped BMP image detected!\n"); + *alloc_addr = dst; return bmp; } #else -bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp) +bmp_image_t *gunzip_bmp(unsigned long addr, unsigned long *lenp, + void **alloc_addr) { return NULL; } @@ -189,11 +201,12 @@ U_BOOT_CMD( static int bmp_info(ulong addr) { bmp_image_t *bmp=(bmp_image_t *)addr; + void *bmp_alloc_addr = NULL; unsigned long len; if (!((bmp->header.signature[0]=='B') && (bmp->header.signature[1]=='M'))) - bmp = gunzip_bmp(addr, &len); + bmp = gunzip_bmp(addr, &len, &bmp_alloc_addr); if (bmp == NULL) { printf("There is no valid bmp file at the given address\n"); @@ -205,8 +218,8 @@ static int bmp_info(ulong addr) printf("Bits per pixel: %d\n", le16_to_cpu(bmp->header.bit_count)); printf("Compression : %d\n", le32_to_cpu(bmp->header.compression)); - if ((unsigned long)bmp != addr) - free(bmp); + if (bmp_alloc_addr) + free(bmp_alloc_addr); return(0); } @@ -225,11 +238,12 @@ int bmp_display(ulong addr, int x, int y) { int ret; bmp_image_t *bmp = (bmp_image_t *)addr; + void *bmp_alloc_addr = NULL; unsigned long len; if (!((bmp->header.signature[0]=='B') && (bmp->header.signature[1]=='M'))) - bmp = gunzip_bmp(addr, &len); + bmp = gunzip_bmp(addr, &len, &bmp_alloc_addr); if (!bmp) { printf("There is no valid bmp file at the given address\n"); @@ -244,8 +258,8 @@ int bmp_display(ulong addr, int x, int y) # error bmp_display() requires CONFIG_LCD or CONFIG_VIDEO #endif - if ((unsigned long)bmp != addr) - free(bmp); + if (bmp_alloc_addr) + free(bmp_alloc_addr); return ret; } diff --git a/include/lcd.h b/include/lcd.h index c6e7fc5..482e606 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -46,7 +46,8 @@ void lcd_initcolregs(void); int lcd_getfgcolor(void); /* gunzip_bmp used if CONFIG_VIDEO_BMP_GZIP */ -struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp); +struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp, + void **alloc_addr); int bmp_display(ulong addr, int x, int y); /**