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

Mike Frysinger
2011-09-23T20:09:17Z

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