Hi Mike,
On Fri, Sep 23, 2011 at 1:09 PM, Mike Frysinger vapier@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