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

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

> > > ...
> > > 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?

Attached is a revised patch, which has a prototype/signature much more 
similar to snprintf(), more appropriate variable types, and improved 
comments/documentation.

I'll plan on committing this sometime late next week...

+------------------+--------------------------+------------------------+
| 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 08:58:59 -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,33 @@ static const struct pci_class pci_class[
 
 DEV_VERBOSE_DEFINE(pci);
 
+/*
+ * Append a formatted string into dest without writing more than len
+ * characters (including the trailing NUL character).
+ *
+ * If vsnprintf() returns an error, *dest is unmodified.  Otherwise,
+ * *dest is updated to point to the last available character in the
+ * buffer (which is occupied by the string-terminating NUL).  It is
+ * not possible to differentiate between an output buffer that is
+ * exactly filled to capacity and one that would have overflowed.
+ */
+static void
+snappendf(char **, size_t, const char * restrict, ...) __printflike(3,4);
+
+static void
+snappendf(char **dest, size_t len, const char * restrict fmt, ...)
+{
+	va_list	ap;
+	int count;
+
+	va_start(ap, fmt);
+	count = vsnprintf(*dest, len, fmt, ap);
+	va_end(ap);
+	if (count < 0 || len == 0)
+		return;
+	*dest += MIN((size_t)count, len - 1);
+}
+
 void
 pci_devinfo(pcireg_t id_reg, pcireg_t class_reg, int showclass, char *cp,
     size_t l)
@@ -612,32 +640,29 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
 		interfacep++;
 	}
 
-	cp += snprintf(cp, ep - cp, "%s %s", vendor, product);
+	snappendf(&cp, ep - cp, "%s %s", vendor, product);
 	if (showclass) {
-		cp += snprintf(cp, ep - cp, " (");
+		snappendf(&cp, ep - cp, " (");
 		if (classp->name == NULL)
-			cp += snprintf(cp, ep - cp,
-			    "class 0x%02x, subclass 0x%02x", pciclass, subclass);
+			snappendf(&cp, ep - cp, "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 - cp, "%s, subclass 0x%02x",
 				    classp->name, subclass);
 			else
-				cp += snprintf(cp, ep - cp, "%s %s",
+				snappendf(&cp, ep - cp, "%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 - cp, ", interface 0x%02x",
+				    interface);
 		} else if (strncmp(interfacep->name, "", 1) != 0)
-			cp += snprintf(cp, ep - cp, ", %s",
-			    interfacep->name);
+			snappendf(&cp, ep - cp, ", %s", interfacep->name);
 		if (revision != 0)
-			cp += snprintf(cp, ep - cp, ", revision 0x%02x",
-			    revision);
-		cp += snprintf(cp, ep - cp, ")");
+			snappendf(&cp, ep - cp, ", revision 0x%02x", revision);
+		snappendf(&cp, ep - cp, ")");
 	}
 }