[U-Boot] [PATCH] tools/mkimage: ignore trailing garbage

[U-Boot] [PATCH] tools/mkimage: ignore trailing garbage

[Peter Korsgaard]

Ignore trailing data after the uimage data and only complain if the
file is too small.

(E.G. if the uImage was stored in flash and hence padded to the flash
sector size).

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 tools/mkimage.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index 967fe9a..58fd20f 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -523,7 +523,14 @@ image_verify_header (char *ptr, int image_size)
 	}
 
 	data = ptr + sizeof(image_header_t);
-	len  = image_size - sizeof(image_header_t) ;
+	len  = ntohl(hdr->ih_size);
+	if (len > (image_size - sizeof(image_header_t))) {
+		fprintf (stderr,
+			"%s: ERROR: \"%s\" is too short (%d vs %d bytes)!\n",
+			 cmdname, imagefile,
+			 image_size - sizeof(image_header_t), len);
+		exit (EXIT_FAILURE);
+	}
 
 	if (crc32 (0, data, len) != ntohl(hdr->ih_dcrc)) {
 		fprintf (stderr,
-- 
1.5.6.5

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Peter Korsgaard]

Use lseek rather than fstat for file size for list mode, so
mkimage -l /dev/mtdblockN works (stat returns st_size == 0 for devices).

Notice that you have to use /dev/mtdblockN and not /dev/mtdN, as the
latter doesn't support mmap.

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 tools/mkimage.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index 58fd20f..ca8254f 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -205,7 +205,8 @@ NXTARG:		;
 		/*
 		 * list header information of existing image
 		 */
-		if (fstat(ifd, &sbuf) < 0) {
+		sbuf.st_size = lseek(ifd, 0, SEEK_END);
+		if (sbuf.st_size == (off_t)-1) {
 			fprintf (stderr, "%s: Can't stat %s: %s\n",
 				cmdname, imagefile, strerror(errno));
 			exit (EXIT_FAILURE);
-- 
1.5.6.5

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Thomas De Schampheleire]

Hi Peter,

On Mon, Dec 1, 2008 at 5:23 PM, Peter Korsgaard <jacmet@sunsite.dk> wrote:
> Use lseek rather than fstat for file size for list mode, so
> mkimage -l /dev/mtdblockN works (stat returns st_size == 0 for devices).
>
> Notice that you have to use /dev/mtdblockN and not /dev/mtdN, as the
> latter doesn't support mmap.
>
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  tools/mkimage.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index 58fd20f..ca8254f 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -205,7 +205,8 @@ NXTARG:             ;
>                /*
>                 * list header information of existing image
>                 */
> -               if (fstat(ifd, &sbuf) < 0) {
> +               sbuf.st_size = lseek(ifd, 0, SEEK_END);
> +               if (sbuf.st_size == (off_t)-1) {
>                        fprintf (stderr, "%s: Can't stat %s: %s\n",
>                                cmdname, imagefile, strerror(errno));

I'd change the error message as well, to be independent of the tool
used to get the file size. For example:
fprintf (stderr, "%s: Can't get size of %s: %s\n",


Best regards,
Thomas

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Peter Korsgaard]

>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

 Thomas> I'd change the error message as well, to be independent of the tool
 Thomas> used to get the file size. For example:
 Thomas> fprintf (stderr, "%s: Can't get size of %s: %s\n",

Ahh yes, good idea.

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Wolfgang Denk]

Dear Peter Korsgaard,

In message <87vdu2tka2.fsf@macbook.be.48ers.dk> you wrote:
> >>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:
> 
>  Thomas> I'd change the error message as well, to be independent of the tool
>  Thomas> used to get the file size. For example:
>  Thomas> fprintf (stderr, "%s: Can't get size of %s: %s\n",
> 
> Ahh yes, good idea.
> 
> From d7c4cb9f290e22d3fc97e43816158c9fd744200c Mon Sep 17 00:00:00 2001
> From: Peter Korsgaard <jacmet@sunsite.dk>
> Date: Mon, 1 Dec 2008 17:13:17 +0100
> Subject: [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option
> 
> Use lseek rather than fstat for file size for list mode, so
> mkimage -l /dev/mtdblockN works (stat returns st_size == 0 for devices).
> 
> Notice that you have to use /dev/mtdblockN and not /dev/mtdN, as the
> latter doesn't support mmap.

Hm... but lseek() on /dev/mtdblockN will return the size of the MTD
device, not of the image that may be stored in it, right?

Later, you should get data checksum errors because of  the  incorrect
lenght,  i.  e.  a  ``ERROR:  "<image>"  has  corrupted data!'' error
message.

I don't think this works.

> -		if (fstat(ifd, &sbuf) < 0) {
> -			fprintf (stderr, "%s: Can't stat %s: %s\n",
> +		sbuf.st_size = lseek(ifd, 0, SEEK_END);
> +		if (sbuf.st_size == (off_t)-1) {
> +			fprintf (stderr, "%s: Can't get size of %s: %s\n",

If you don't use *stat(), you should not use any struct stat type
either.

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 at denx.de
Der Horizont vieler Menschen ist ein Kreis mit Radius Null --
und das nennen sie ihren Standpunkt.

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Peter Korsgaard]

>>>>> "Wolfgang" == Wolfgang Denk <wd@denx.de> writes:

Hi,

 Wolfgang> Hm... but lseek() on /dev/mtdblockN will return the size of
 Wolfgang> the MTD device, not of the image that may be stored in it,
 Wolfgang> right?

Yes.

 Wolfgang> Later, you should get data checksum errors because of the
 Wolfgang> incorrect lenght, i.  e.  a ``ERROR: "<image>" has
 Wolfgang> corrupted data!'' error message.

Exactly, hence my other patch (tools/mkimage: ignore trailing garbage)
to only error out if the size is smaller than the size defined in the
header.

 Wolfgang> I don't think this works.

 >> -		if (fstat(ifd, &sbuf) < 0) {
 >> -			fprintf (stderr, "%s: Can't stat %s: %s\n",
 >> +		sbuf.st_size = lseek(ifd, 0, SEEK_END);
 >> +		if (sbuf.st_size == (off_t)-1) {
 >> +			fprintf (stderr, "%s: Can't get size of %s: %s\n",

 Wolfgang> If you don't use *stat(), you should not use any struct
 Wolfgang> stat type either.

Ok, I kept it like that to keep the patch as minimal as possible -
I'll change it and resend.

-- 
Bye, Peter Korsgaard

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Wolfgang Denk]

Dear Peter Korsgaard,

In message <87ljuxzosn.fsf@macbook.be.48ers.dk> you wrote:
> 
>  Wolfgang> Later, you should get data checksum errors because of the
>  Wolfgang> incorrect lenght, i.  e.  a ``ERROR: "<image>" has
>  Wolfgang> corrupted data!'' error message.
> 
> Exactly, hence my other patch (tools/mkimage: ignore trailing garbage)
> to only error out if the size is smaller than the size defined in the
> header.

This doesn't make sense to me. If we have a way to check the image, we
want to check everything, including correct image length.

>  Wolfgang> If you don't use *stat(), you should not use any struct
>  Wolfgang> stat type either.
> 
> Ok, I kept it like that to keep the patch as minimal as possible -
> I'll change it and resend.

I think it makes little sense as my feeling is the changes make
mkimage checking not better, just worse.

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 at denx.de
Honest error is to be pitied, not ridiculed.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH] tools/mkimage: use lseek rather than fstat for file size for -l option

[Peter Korsgaard]

>>>>> "Wolfgang" == Wolfgang Denk <wd@denx.de> writes:

Hi,

 >> Exactly, hence my other patch (tools/mkimage: ignore trailing
 >> garbage) to only error out if the size is smaller than the size
 >> defined in the header.

 Wolfgang> This doesn't make sense to me. If we have a way to check
 Wolfgang> the image, we want to check everything, including correct
 Wolfgang> image length.

I provided a reason for the change in the patch:

"E.G. if the uImage was stored in flash and hence padded to the flash
sector size"

I can add a warning if there's trailing garbage if you prefer, but I
think the use case is valid.

 >> Ok, I kept it like that to keep the patch as minimal as possible -
 >> I'll change it and resend.

 Wolfgang> I think it makes little sense as my feeling is the changes make
 Wolfgang> mkimage checking not better, just worse.

Ok, fixed in the v2 patch I recently sent.

-- 
Bye, Peter Korsgaard