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