Patchwork [U-Boot] lcd: align bmp header when uncopmressing image

login
register
mail settings
Submitter Piotr Wilczek
Date May 31, 2013, 11:26 a.m.
Message ID <1369999573-15449-1-git-send-email-p.wilczek@samsung.com>
Download mbox | patch
Permalink /patch/247928/
State Superseded
Delegated to: Anatolij Gustschin
Headers show

Comments

Piotr Wilczek - May 31, 2013, 11:26 a.m.
When compressed image is loaded, it must be decompressed
to an aligned address + 2 to avoid unaligned access exception
on some ARM platforms.

Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Anatolij Gustschin <agust@denx.de>
CC: Wolfgang Denk: <wd@denx.de>
---
 common/cmd_bmp.c |   40 +++++++++++++++++++++++++++-------------
 include/lcd.h    |    3 ++-
 2 files changed, 29 insertions(+), 14 deletions(-)
Wolfgang Denk - May 31, 2013, 2:22 p.m.
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
Nikita Kiryanov - June 2, 2013, 11:05 a.m.
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");
>
Piotr Wilczek - June 3, 2013, 6:17 a.m.
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
Piotr Wilczek - June 3, 2013, 6:39 a.m.
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
Wolfgang Denk - June 3, 2013, 11:15 a.m.
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
Wolfgang Denk - June 3, 2013, 11:18 a.m.
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
Piotr Wilczek - June 3, 2013, 2:14 p.m.
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
Wolfgang Denk - June 3, 2013, 6:45 p.m.
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

Patch

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);
 
 /**