[U-Boot] [PATCH 4/4] Use snprintf() in network code

[Mike Frysinger]

On Friday, September 23, 2011 14:30:09 Simon Glass wrote:
> On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote:
> > On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
> >> --- a/net/eth.c
> >> +++ b/net/eth.c
> >> 
> >>       char buf[20];
> >> -     sprintf(buf, "%pM", enetaddr);
> >> +     snprintf(buf, sizeof(buf), "%pM", enetaddr);
> > 
> > a mac address will not take more than 19 bytes.  unless the sprintf code
> > is completely busted, but if that's the case, we should fix that instead
> > since it'd be pretty fundamentally screwed.
> 
> OK (18 bytes?)

right ... i meant there's 19 available (since 20 makes NUL), and there's no 
way it should hit that 19 limit.

> >>       char enetvar[32];
> >> -     sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> >> +     snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
> >> +              base_name, index);
> > 
> > in order for this to overflow, we have to have 1000+ eth devices (maybe
> > more? i'd have to read the code closer)
> 
> Or base_name could be longer.

true, but base_name atm is supposed to be "eth" or "usbeth".  i would hope 
we'd be diligent about device naming conventions rather than letting someone 
slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]).

> >>       char enetvar[15];
> >> -     sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
> >> +     snprintf(enetvar, sizeof(enetvar),
> >> +             index ? "eth%dmacskip" : "ethmacskip", index);
> > 
> > in order for this to overflow, we have to have 10000+ eth devices
> > 
> > please look at the realistic needs rather than blanket converting to
> > snprintf
> 
> OK, this is fair enough. There are a couple of functions like
> ip_to_string() which don't get passed a length. I would prefer to see
> this done, so we can use snprintf() or assert() since the code then
> becomes more robust. But there are no bugs there at present. Thoughts?

for funcs that do generally handle arbitrary data, i don't mind including len 
specs, but when we're using known standards, i'd prefer to stick to the 
smaller/simpler code.  this is a thin boot loader that is meant to be fast, 
and there are plenty of ways to bring down the system otherwise (and i don't 
think these cases are generally a robustness concern).

don't know if wolfgang has an opinion.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110923/7c5e56f6/attachment.pgp