[U-Boot-Users] [PATCH v3] Fix initrd length miscalculation in bootm command when using a dtu

[Wolfgang Denk]

Dear Timur,

in message <463A88BA.2030607 at freescale.com> you wrote:
> 
> > So before your patch we had "len", "checksum", and "data", and now we
> > have "len", "data", and "checksum".
> > 
> > What is your reason for this change? I see  no  functinal  difference
> > nor any improvement of readability.
> 
> It's for the comment that I added.  I wanted to make sure that the reader understood what 
> 'len' and 'data' are for, since their purpose is not obvious from just reading the code.

Your comment is actually wrong. The use of "len" is not limited to
that purpose.

But that doesn't mean that you can overwrite  this  variable  without
checking.

> >> -		if (crc32 (0, (uchar *)data, len) != checksum) {
> >> +		if (crc32 (0, (uchar *) &header, sizeof(image_header_t)) != checksum) {
> > 
> > This looks functionally identical to me, but is less readable.
> 
> The point behind the patch is to make sure that 'len' and 'data' are only used for the 
> purpose they are intended.  Here, len and data are used as temporary variables.

...and here this is perfectly legal, as  "len"  does  not  store  any
other value that is needed later.

> >> @@ -779,9 +776,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
> >>
> >>  			checksum = ntohl(hdr->ih_dcrc);
> >>  			addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
> >> -			len = ntohl(hdr->ih_size);
> >>
> >> -			if(checksum != crc32(0, (uchar *)addr, len)) {
> >> +			if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {
> > 
> > Same here. functionally identical but less readable.
> 
> Not true.  In this case, len has already been assigned the length of the initrd, and here 
> we lose this value.  Without this change, the kernel will panic at boot.

Right. I agree that this is the core of the problem,  but  from  your
patch  it was impossible to see. You have to read the diff, plus your
patch description, plus the code.


I think we should just fix the bug, which is the incorrect use of the
variable "len" at a place where it was already used. As Johns  Daniel
pointed  out  (see his message from Tue, 27 Mar 2007 11:25:55 -0500),
this is the core of your poatch and all that needs to be changed.

> > With this patch it is impossible to see what the problem is  you  are
> > trying to fix.
> 
> It's clearly explained in the comment: 'data' and 'len' are the address and length of the 
> initrd, so this patch makes sure that these variables only contain those values.

No, this explanation is wrong. "len" gets used for some other
purposes, too.


To put this problem finally to rest I checked in the fix as suggested
by Johns Daniel, i. e. the core idea of your patch. See
http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commit;h=9877d7dcd1eebe61aa5d8b8ffe9c048ea426e6f6


Thanks for pointing out the problem and providing a fix.

And please accept my apologies thatt his was so complicated and  took
so  long.  [Nevertheless you still might want to try to find a way to
access the repository I  created  for  you  in  case  you  have  more
patches.]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,    CEO: 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
"My name is Linus Torvalds, you messed with my kernel, prepare to die"
                                                  - Linus Torvalds in
      <Pine.LNX.3.91.960426110644.24860I-100000 at linux.cs.Helsinki.FI>