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

[Simon Glass]

Hi Mike,

On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Friday, September 23, 2011 13:38:52 Simon Glass wrote:
>> This tidies up network code to use snprintf() in most cases instead of
>> sprintf(). A few functions remain as they require header file changes.
>
> NAK to most of these.  we pick local sized buffers that are known to not
> overflow, or require circumstances that aren't really feasible.

Yes that's why I sent this patch on the end, to see how far this
should be taken.

>
> 3 examples (which are the first 3 changes in this patch) below ...
>
>> --- 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?)

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

>
>>       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?

Regards,
Simon

> -mike
>