Replace the 'p += snprintf' idiom with a bounded append

spine.c

@@ -645,25 +645,25 @@ int main(int argc, char *argv[]) {
 
 	/* obtain the list of hosts to poll */
 	{
-		int remaining = MEGA_BUFSIZE - (qp - querybuf);
-		qp += snprintf(qp, remaining, "SELECT SQL_NO_CACHE id, device_threads, picount, picount/device_threads AS tppi FROM host AS h LEFT JOIN (SELECT host_id, COUNT(*) AS picount FROM poller_item GROUP BY host_id) AS pi ON h.id = pi.host_id");
+		size_t remaining = MEGA_BUFSIZE - (qp - querybuf);
+		spine_appendf(&qp, &remaining, "SELECT SQL_NO_CACHE id, device_threads, picount, picount/device_threads AS tppi FROM host AS h LEFT JOIN (SELECT host_id, COUNT(*) AS picount FROM poller_item GROUP BY host_id) AS pi ON h.id = pi.host_id");
 		remaining = MEGA_BUFSIZE - (qp - querybuf);
-		qp += snprintf(qp, remaining, " WHERE disabled = ''");
+		spine_appendf(&qp, &remaining, " WHERE disabled = ''");
 
 		remaining = MEGA_BUFSIZE - (qp - querybuf);
-		qp += snprintf(qp, remaining, " AND availability_method != %d", AVAIL_STREAM);
+		spine_appendf(&qp, &remaining, " AND availability_method != %d", AVAIL_STREAM);
 
 		if (!strlen(set.host_id_list)) {
 			qp += append_hostrange(qp, "h.id");	/* AND id BETWEEN a AND b */
 		} else {
 			remaining = MEGA_BUFSIZE - (qp - querybuf);
-			qp += snprintf(qp, remaining, " AND h.id IN(%s)", set.host_id_list);
+			spine_appendf(&qp, &remaining, " AND h.id IN(%s)", set.host_id_list);
 		}
 
 		remaining = MEGA_BUFSIZE - (qp - querybuf);
-		qp += snprintf(qp, remaining, " AND h.poller_id = %i", set.poller_id);
+		spine_appendf(&qp, &remaining, " AND h.poller_id = %i", set.poller_id);
 		remaining = MEGA_BUFSIZE - (qp - querybuf);
-		qp += snprintf(qp, remaining, " ORDER BY picount DESC");
+		spine_appendf(&qp, &remaining, " ORDER BY picount DESC");
 	}
 
 	SPINE_LOG_DEVDBG(("DEVDBG: Host SQL:%s", querybuf));

tests/unit/test_linked.c

@@ -20,6 +20,8 @@
 #include "util.h"
 #include "ping.h"
 
+#include <stddef.h>
+
 /* provided by tests/fuzz/stubs.c, as spine.c would */
 extern int *debug_devices;
 
@@ -457,6 +459,156 @@ static void test_is_debug_device_matches_only_listed_ids(void **state) {
 	debug_devices = saved;
 }
 
+
+/* ---------------------------------------------------------------------------
+ * spine_appendf (util.c)
+ *
+ * Replaces `p += snprintf(p, remaining, ...)`, which advances by the length
+ * snprintf *would* have written, so the first truncation puts the cursor past
+ * the end and the next `remaining` underflows to a huge size_t.
+ * ------------------------------------------------------------------------- */
+
+struct guarded_buf {
+	char body[32];
+	char canary[8];
+};
+
+static void guarded_init(struct guarded_buf *g) {
+	memset(g->body, 0, sizeof(g->body));
+	memset(g->canary, 0x7e, sizeof(g->canary));
+}
+
+static void guarded_check(struct guarded_buf *g) {
+	size_t i;
+
+	for (i = 0; i < sizeof(g->canary); i++) {
+		assert_int_equal((unsigned char) g->canary[i], 0x7e);
+	}
+}
+
+static void test_appendf_writes_and_advances(void **state) {
+	struct guarded_buf g;
+	char *p;
+	size_t remaining;
+
+	(void) state;
+	guarded_init(&g);
+	p = g.body;
+	remaining = sizeof(g.body);
+
+	assert_true(spine_appendf(&p, &remaining, "abc"));
+	assert_int_equal(p - g.body, 3);
+	assert_int_equal(remaining, sizeof(g.body) - 3);
+	assert_string_equal(g.body, "abc");
+	guarded_check(&g);
+}
+
+static void test_appendf_accumulates(void **state) {
+	struct guarded_buf g;
+	char *p;
+	size_t remaining;
+
+	(void) state;
+	guarded_init(&g);
+	p = g.body;
+	remaining = sizeof(g.body);
+
+	assert_true(spine_appendf(&p, &remaining, "SELECT %d", 7));
+	assert_true(spine_appendf(&p, &remaining, " FROM %s", "t"));
+	assert_string_equal(g.body, "SELECT 7 FROM t");
+	assert_int_equal(remaining, sizeof(g.body) - strlen("SELECT 7 FROM t"));
+	guarded_check(&g);
+}
+
+/* The case the old idiom got wrong. */
+static void test_appendf_reports_truncation_and_stays_in_bounds(void **state) {
+	struct guarded_buf g;
+	char *p;
+	size_t remaining;
+
+	(void) state;
+	guarded_init(&g);
+	p = g.body;
+	remaining = sizeof(g.body);
+
+	assert_false(spine_appendf(&p, &remaining, "%s", "0123456789012345678901234567890123456789"));
+
+	/* cursor lands on the terminator, not past the end */
+	assert_true(p >= g.body);
+	assert_true(p < g.body + sizeof(g.body));
+	assert_int_equal(*p, '\0');
+	assert_int_equal(remaining, 1);
+	assert_int_equal(strlen(g.body), sizeof(g.body) - 1);
+	guarded_check(&g);
+}
+
+static void test_appendf_after_truncation_keeps_failing(void **state) {
+	struct guarded_buf g;
+	char *p;
+	size_t remaining;
+	char full[sizeof(g.body)];
+
+	(void) state;
+	guarded_init(&g);
+	p = g.body;
+	remaining = sizeof(g.body);
+
+	assert_false(spine_appendf(&p, &remaining, "%s", "0123456789012345678901234567890123456789"));
+	memcpy(full, g.body, sizeof(full));
+
+	/* a second append must not write anything, anywhere */
+	assert_false(spine_appendf(&p, &remaining, " AND poller_id=%d", 3));
+	assert_memory_equal(g.body, full, sizeof(full));
+	guarded_check(&g);
+}
+
+static void test_appendf_rejects_null_arguments(void **state) {
+	char buf[8] = "";
+	char *p = buf;
+	size_t remaining = sizeof(buf);
+	char *nullp = NULL;
+
+	(void) state;
+
+	assert_false(spine_appendf(NULL, &remaining, "x"));
+	assert_false(spine_appendf(&nullp, &remaining, "x"));
+	assert_false(spine_appendf(&p, NULL, "x"));
+}
+
+static void test_appendf_rejects_an_exhausted_buffer(void **state) {
+	char buf[8] = "";
+	char *p = buf;
+	size_t remaining = 0;
+
+	(void) state;
+
+	assert_false(spine_appendf(&p, &remaining, "x"));
+	assert_ptr_equal(p, buf);
+	assert_int_equal(buf[0], '\0');
+}
+
+/* Documents the defect: the same sequence with the old idiom leaves the cursor
+   outside the buffer, so the next remainder is negative. */
+static void test_old_idiom_overshoots_where_appendf_does_not(void **state) {
+	char buf[32];
+	char *p = buf;
+	ptrdiff_t old_offset;
+	char *q;
+	size_t remaining;
+
+	(void) state;
+
+	p += snprintf(p, sizeof(buf), "%s", "0123456789012345678901234567890123456789");
+	old_offset = p - buf;
+	assert_true(old_offset > (ptrdiff_t) sizeof(buf));
+	assert_true((ptrdiff_t) (sizeof(buf) - old_offset) < 0);
+
+	q = buf;
+	remaining = sizeof(buf);
+	assert_false(spine_appendf(&q, &remaining, "%s", "0123456789012345678901234567890123456789"));
+	assert_true(q - buf < (ptrdiff_t) sizeof(buf));
+}
+
 int main(void) {
 	const struct CMUnitTest tests[] = {
 		cmocka_unit_test(test_strncopy_truncates_within_the_buffer),
@@ -496,6 +648,13 @@ int main(void) {
 		cmocka_unit_test(test_get_date_format_clamps_an_out_of_range_format),
 		cmocka_unit_test(test_get_date_format_covers_each_supported_format),
 		cmocka_unit_test(test_is_debug_device_matches_only_listed_ids),
+		cmocka_unit_test(test_appendf_writes_and_advances),
+		cmocka_unit_test(test_appendf_accumulates),
+		cmocka_unit_test(test_appendf_reports_truncation_and_stays_in_bounds),
+		cmocka_unit_test(test_appendf_after_truncation_keeps_failing),
+		cmocka_unit_test(test_appendf_rejects_null_arguments),
+		cmocka_unit_test(test_appendf_rejects_an_exhausted_buffer),
+		cmocka_unit_test(test_old_idiom_overshoots_where_appendf_does_not),
 	};
 
 	return cmocka_run_group_tests(tests, NULL, NULL);