Subject: Re: CVS commit: src/sys
To: None <kleink@mibh.de>
From: Jun-ichiro itojun Hagino <itojun@itojun.org>
List: tech-kern
Date: 04/25/2004 02:06:35
> I've cleaned up after this change (which involved bumping the kernel
> version, updating libpci and bumping its major, and updating assorted
> documentation).
> 
> While doing so I've also had a look at the change itself (I do support
> this kind of interface change), and I've found its quality rather
> questionable for a change that has been conducted to achieve:
> 
>         "basically code sweep and promoting better practice.
>          i see scary ones here and there."[1]
> 
> One such example, taken from the change to pci_devinfo(), is:
> 
> +       char *ep;
> +
> +       ep = cp + l;
> 
> [...]
> 
> -               cp += sprintf(cp, "%svendor 0x%04x product 0x%04x",
> +               cp += snprintf(cp, ep - cp, "%svendor 0x%04x product 0x%04x",
>                     unmatched, vendor, product);
> 
> snprintf() returns the amount of storage _required_ to format the
> arguments given, which may be larger than the available storage size
> passed to it (ep - cp).  In a worst case scenario, the storage will
> not be sufficient, and the value returned will cause cp to overflow
> past the end of the storage as given by ep.  In subsequent calls to
> snprintf(), (ep - cp) will be a negative value which, when passed to
> snprintf() (size_t!), will be treated as a large unsigned number, this
> time causing storage past the end of the buffer to be overwritten.

	the original code (with sprintf) is already broken, as sprintf()
	returns -1 on failure.  we just need to fix all of these
	cp += sprintf (or snprintf).

itojun