Re: [PATCH] ofwboot bootp/bootparams mixup

[Roy Marples <roy%marples.name@localhost>]

Daniele Forsi wrote:
> 2009/5/4 Roy Marples>:
> > Christos Zoulas wrote:
> > > In article <49FEC7F2.1070708%marples.name@localhost>,
> > > >              }
> > > > -            if (tag == TAG_ROOTPATH) {
> > > > +            if (tag == TAG_ROOTPATH && size <= sizeof(rootpath)) {
> > > >                      strncpy(rootpath, (char *)cp, sizeof(rootpath));
> > > >                      rootpath[size] = '\0';
> > > >              }
> > > > -            if (tag == TAG_HOSTNAME) {
> > > > +            if (tag == TAG_HOSTNAME && size <= sizeof(hostname)) {
> > > >                      strncpy(hostname, (char *)cp, sizeof(hostname));
> > > >                      hostname[size] = '\0';
> > > The <= test should really be < I think.
> > Yes, good catch.
>
> isn't it redundant checking size < sizeof(hostname) before using
> strncpy() with the same argument and terminating the string? In that
> case the string is always terminated.

Using a truncated hostname would be incorrect.
We could drop the hostname[size] = '\0'; though as strncpy will do that 
for us.

> How about not checking size < sizeof(hostname) and replacing
> hostname[size] = '\0'; with hostname[sizeof(hostname) - 1] = '\0';
>
> > -               if (tag == TAG_SWAPSERVER) {
> > +               if (tag == TAG_SWAPSERVER && size >= sizeof(rootip.s_addr)) {
> >                        /* let it override bp_siaddr */
> >                        (void)memcpy(&rootip.s_addr, cp, sizeof(rootip.s_addr));
> >                }
>
> I might be missing something, but why copying when size is bigger?
> Shouldn't it be <= ?

The spec allows for an array of IP addresses even though the option 
could only demand one. We only work with one, so we discard the rest. 
However, we do need to ensure that we have enough data for one address.

Thanks

Roy