Hi Mike,
On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger vapier@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