From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Date: Fri, 23 Sep 2011 16:09:17 -0400 Subject: [U-Boot] [PATCH 4/4] Use snprintf() in network code In-Reply-To: References: <1316799532-20761-1-git-send-email-sjg@chromium.org> <201109231415.27466.vapier@gentoo.org> Message-ID: <201109231609.18144.vapier@gentoo.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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