OS-4516 strlist.c in pointer arithmetic blunder

src/common/strings/Makefile

@@ -38,6 +38,7 @@ TEST_PROGRAMS = \
 		test1 \
 		test2 \
 		test3 \
+		test4 \
 		testpath
 
 TEST_BINS =	$(TEST_PROGRAMS:%=obj/%)
@@ -55,7 +56,7 @@ CTFMERGE =	/usr/bin/true
 CC =		$(GCC)
 CSTYLE =	../../../tools/cstyle
 
-LIBS =		-lcmdutils
+LIBS =		-lcmdutils -lumem
 
 COMPILE_C =	$(CC) $(CPPFLAGS) $(LDFLAGS) $(CFLAGS) -c -o $@ $< $(LIBS)
 
@@ -92,7 +93,8 @@ test: all $(TEST_PROGRAMS:%=%.runtest)
 # Compilation and linking targets
 #
 obj/test%: obj/test%.o $(OBJECTS)
-	$(CC) $(CPPFLAGS) $(LDFLAGS) $(CFLAGS) -o $@ $^ $(LIBS)
+	$(CC) $(CPPFLAGS) $(LDFLAGS) $(CFLAGS) -o $@ $^ $(LIBS) \
+	    tests/force_umem_debug.c
 	$(CTFMERGE) -L VERSION -o $@ $(OBJECTS)
 
 obj/test%.o: tests/test%.c

src/common/strings/strlist.c

@@ -35,10 +35,13 @@
 
 /*
  * The capacity of the list is expressed as an unsigned int, but the list
- * itself is backed by a contiguous array of pointers to strings.  That
- * array is bounded above by the maximum size we can allocate and address:
+ * itself is backed by a contiguous array of pointers to strings.  The backing
+ * array includes one extra element beyond the usable capacity, such that the
+ * array is always terminated by a NULL pointer.  This definition accounts for
+ * the extra element, as well as the maximum byte length we can allocate and
+ * address with a size_t.
  */
-#define	MAX_CAPACITY	(SIZE_MAX / sizeof (char *))
+#define	MAX_CAPACITY	(SIZE_MAX / sizeof (char *) - 1)
 
 
 struct strlist {
@@ -77,14 +80,15 @@ static int
 strlist_grow_by(strlist_t *sl, unsigned int grow_by)
 {
 	char **new_strings = NULL;
-	unsigned int new_capacity = sl->sl_capacity + grow_by;
+	unsigned int old_capacity = sl->sl_capacity;
+	unsigned int new_capacity = old_capacity + grow_by;
 	/*
 	 * The new size must include the extra array element for the NULL
 	 * terminator, but the old size does not; a new NULL terminator will be
 	 * written into the new object by memset(), rather than copied from the
 	 * original.
 	 */
-	size_t oldsz = sl->sl_capacity * sizeof (char *);
+	size_t oldsz = old_capacity * sizeof (char *);
 	size_t newsz = (1 + new_capacity) * sizeof (char *);
 
 	if (grow_by == 0) {
@@ -95,7 +99,7 @@ strlist_grow_by(strlist_t *sl, unsigned int grow_by)
 	 * Check that the growth size does not cause us to exceed the maximum
 	 * possible capacity, with care to avoid integer overflow:
 	 */
-	if (grow_by > (MAX_CAPACITY - sl->sl_capacity)) {
+	if (grow_by > (MAX_CAPACITY - old_capacity)) {
 		errno = ENOSPC;
 		return (-1);
 	}
@@ -105,7 +109,7 @@ strlist_grow_by(strlist_t *sl, unsigned int grow_by)
 	}
 
 	(void) memcpy(new_strings, sl->sl_strings, oldsz);
-	(void) memset(new_strings + oldsz, 0, newsz - oldsz);
+	(void) memset(&new_strings[old_capacity], 0, newsz - oldsz);
 
 	free(sl->sl_strings);
 

src/common/strings/tests/force_umem_debug.c

@@ -0,0 +1,26 @@
+/*
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
+ *
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
+ */
+
+/*
+ * Copyright 2015 Joyent, Inc.
+ */
+
+const char *
+_umem_debug_init()
+{
+	return ("default,verbose");
+}
+
+const char *
+_umem_logging_init(void)
+{
+	return ("fail,contents");
+}

src/common/strings/tests/test4.c

@@ -0,0 +1,87 @@
+/*
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
+ *
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
+ */
+
+/*
+ * Copyright 2015 Joyent, Inc.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <strings.h>
+#include <err.h>
+#include <errno.h>
+#include <sys/debug.h>
+
+#include "strlist.h"
+
+
+#define	NUM_LISTS	128
+#define	NUM_STRINGS	290
+
+
+void
+append_string(strlist_t *sl, const char *s)
+{
+	if (strlist_set_tail(sl, s) != 0) {
+		err(1, "strlist_set_tail failure");
+	}
+}
+
+int
+main(int argc, char *argv[])
+{
+	strlist_t *sl[NUM_LISTS];
+
+	fprintf(stdout, "alloc...\n");
+	for (unsigned int i = 0; i < NUM_LISTS; i++) {
+		if (strlist_alloc(&sl[i], 0) != 0) {
+			err(1, "[%3u] strlist_alloc(, 0) failure", i);
+		}
+	}
+	fprintf(stdout, "done.\n");
+
+	fprintf(stdout, "first append pass...\n");
+	for (unsigned int i = 0; i < NUM_STRINGS; i++) {
+		for (unsigned int j = 0; j < NUM_LISTS; j++) {
+			append_string(sl[j], "|sample string 0 sample string 1"
+			    " sample string 2 sample string 3|");
+		}
+	}
+	fprintf(stdout, "done.\n");
+
+	fprintf(stdout, "reset...\n");
+	for (unsigned int i = 0; i < NUM_LISTS; i++) {
+		strlist_reset(sl[i]);
+	}
+	fprintf(stdout, "done.\n");
+
+	fprintf(stdout, "second append pass...\n");
+	for (unsigned int i = 0; i < NUM_STRINGS; i++) {
+		for (unsigned int j = 0; j < NUM_LISTS; j++) {
+			append_string(sl[j], "|sample string 0 sample string 1"
+			    " sample string 2 sample string 3|");
+		}
+	}
+	fprintf(stdout, "done.\n");
+
+	fprintf(stdout, "free...\n");
+	for (unsigned int i = 0; i < NUM_LISTS; i++) {
+		strlist_free(sl[i]);
+	}
+	fprintf(stdout, "done.\n");
+
+	if (getenv("ABORT_ON_EXIT") != NULL) {
+		abort();
+	}
+
+	return (0);
+}