Re: CVS commit: src/sys/dev/pci

[Paul Goyette <paul%whooppee.com@localhost>]

> > ... I think that perhaps a variant that returns the
> > number of characters appended would be more useful. like
> > if (len >= max)
> > 	dest = end;
> > else
> > 	dest += len;
> >
> > but, I would leave the whole thing alone instead :-)
> > or write a function that appends and moves the pointer, like
> >
> > snappendprintf(char **dest, const char *end, fmt, ...)

snappendprintf() seemed a rather unwieldly function name, so I called it 
snappendf()!

The attached diffs implement this function and use in throughout 
pci_devinfo().


At some time in the future we will eventually get products whose verbose 
description exceed the currently-allocated 256-byte buffer, so I really 
think we should implement this (or something similar) to protect against 
buffer overflows.  Does anyone have any major objections?  Are there 
better alternatives than snappendf() that should be pursued instead?



+------------------+--------------------------+------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:      |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+

Index: pci_subr.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pci_subr.c,v
retrieving revision 1.152
diff -u -p -r1.152 pci_subr.c
--- pci_subr.c	20 Oct 2016 04:11:02 -0000	1.152
+++ pci_subr.c	28 Oct 2016 05:07:41 -0000
@@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: pci_subr.c,v
 #include <sys/module.h>
 #else
 #include <pci.h>
+#include <stdarg.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -567,6 +568,25 @@ static const struct pci_class pci_class[
 
 DEV_VERBOSE_DEFINE(pci);
 
+/*
+ * Append a formatted string into dest without writing beyond end, and
+ * update dest to point to next available character position
+ */
+static void
+snappendf(char **dest, const char *end, const char * restrict fmt, ...)
+{
+	va_list	ap;
+	int count;
+	int len = end - *dest;
+
+	va_start(ap, fmt);
+	count = vsnprintf(*dest, len, fmt, ap);
+	va_end(ap);
+	if (count > len)
+		count = len;
+	*dest += count;
+}
+
 void
 pci_devinfo(pcireg_t id_reg, pcireg_t class_reg, int showclass, char *cp,
     size_t l)
@@ -612,32 +632,29 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
 		interfacep++;
 	}
 
-	cp += snprintf(cp, ep - cp, "%s %s", vendor, product);
+	snappendf(&cp, ep, "%s %s", vendor, product);
 	if (showclass) {
-		cp += snprintf(cp, ep - cp, " (");
+		snappendf(&cp, ep, " (");
 		if (classp->name == NULL)
-			cp += snprintf(cp, ep - cp,
-			    "class 0x%02x, subclass 0x%02x", pciclass, subclass);
+			snappendf(&cp, ep, "class 0x%02x, subclass 0x%02x",
+			    pciclass, subclass);
 		else {
 			if (subclassp == NULL || subclassp->name == NULL)
-				cp += snprintf(cp, ep - cp,
-				    "%s, subclass 0x%02x",
+				snappendf(&cp, ep, "%s, subclass 0x%02x",
 				    classp->name, subclass);
 			else
-				cp += snprintf(cp, ep - cp, "%s %s",
+				snappendf(&cp, ep, "%s %s",
 				    subclassp->name, classp->name);
 		}
 		if ((interfacep == NULL) || (interfacep->name == NULL)) {
 			if (interface != 0)
-				cp += snprintf(cp, ep - cp,
-				    ", interface 0x%02x", interface);
+				snappendf(&cp, ep, ", interface 0x%02x",
+				    interface);
 		} else if (strncmp(interfacep->name, "", 1) != 0)
-			cp += snprintf(cp, ep - cp, ", %s",
-			    interfacep->name);
+			snappendf(&cp, ep, ", %s", interfacep->name);
 		if (revision != 0)
-			cp += snprintf(cp, ep - cp, ", revision 0x%02x",
-			    revision);
-		cp += snprintf(cp, ep - cp, ")");
+			snappendf(&cp, ep, ", revision 0x%02x", revision);
+		snappendf(&cp, ep, ")");
 	}
 }