On Saturday 24 April 2004 04:58, matthew green wrote:

[moved from source-changes to tech-kern]

> 
>    Modified Files:
>    	src/sys/arch/algor/pci: pcib.c
>    	src/sys/arch/alpha/pci: sio.c
>    	src/sys/arch/amd64/pci: pchb.c pcib.c
>    	src/sys/arch/bebox/pci: pchb.c pcib.c
>    	src/sys/arch/cats/pci: pcib.c
>    	src/sys/arch/cobalt/pci: pcib.c
>    	src/sys/arch/i386/pci: p64h2apic.c pceb.c pchb.c pcib.c pcmb.c
>    	src/sys/arch/ibmnws/pci: pchb.c pcib.c
>    	src/sys/arch/macppc/pci: pchb.c
>    	src/sys/arch/mvmeppc/pci: pchb.c pcib.c
>    	src/sys/arch/netwinder/pci: pcib.c
>    	src/sys/arch/pmppc/pci: pchb.c
>    	src/sys/arch/prep/pci: gten.c pchb.c pcib.c
>    	src/sys/arch/sandpoint/pci: pchb.c pcib.c
>    	src/sys/arch/x86/pci: aapic.c
>    	src/sys/dev/cardbus: cardbus.c com_cardbus.c ehci_cardbus.c
>    	    fwohci_cardbus.c ohci_cardbus.c rbus_ppb.c
>    	src/sys/dev/pci: autri.c cmpci.c cs4280.c cs4281.c eap.c ehci_pci.c
>    	    emuxki.c esa.c esm.c fwlynx_pci.c fwohci_pci.c gtp.c if_an_pci.c
>    	    if_de.c if_gem_pci.c if_mtd_pci.c if_txp.c if_wi_pci.c igsfb_pci.c
>    	    iha_pci.c joy_pci.c machfb.c ohci_pci.c pccbb.c pci.c pci_subr.c
>    	    pciide_common.c pcivar.h pcscp.c ppb.c uhci_pci.c vga_pci.c yds.c
>    
>    Log Message:
>    pass string length (= boundary info) to pci_devinfo so that we do not run over
>    the end of memory region
> 
> 
> this requires a kernel bump i think?

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.


So again, while I do support this kind of interface change, it's
obvious that much more care needs to be applied when making it, and it
again demonstrates than merely substituting a function perceived as
"safe" can create a false sense of security, accomplishing little else.



- Klaus

P.S. Please fix this. :-)

[1] http://mail-index.netbsd.org/source-changes/2004/04/22/0000.html