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

[Simon Glass]

Hi Mike,

On Fri, Sep 23, 2011 at 1:09 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> 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 ;]).

Well here I feel that it is pushing it a bit to have a function that
builds a string based on arguments over which it has no control. I
suppose an assert(strlen(base_name) < 23) or whatever would do it. But
isn't snprintf() better?

>
>> >>       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).

Well that argues towards using assert() in these cases rather than
just ignoring them. I'm keen to try to make U-Boot more testable. I do
understand it is just a 'thin' boot loader, but it has quite a few
features now, and it if breaks, the system won't boot.

I do agree there is a difference between telling devs about a bug /
overflow at build/test time and bloating the code in production.
Either/or is good with me, but I'm not entirely comfortable with just
ignoring these issues and hoping they don't bite us. It's not like we
have a test suite to check it.

>
> don't know if wolfgang has an opinion.

<insert here>

Regards,
Simon

> -mike
>