Merge branch 'cleanups' into 'main'

Miscellaneous cleanups

See merge request wayland/wayland!414

src/connection.c

@@ -91,19 +91,45 @@ ring_buffer_mask(const struct wl_ring_buffer *b, size_t i) {
 	return i & m;
 }
 
+static size_t
+ring_buffer_size(struct wl_ring_buffer *b)
+{
+	return b->head - b->tail;
+}
+
+/* Precondition: the data will not overflow the buffer */
 static int
 ring_buffer_put(struct wl_ring_buffer *b, const void *data, size_t count)
 {
-	size_t head, size;
+	size_t head, size, buffer_size, capacity;
+
+	if (b->head < b->tail) {
+		wl_abort("ring_buffer_put: ring buffer corrupt, %zu < %zu\n",
+		         b->head, b->tail);
+	}
+
+	capacity = ring_buffer_capacity(b);
+	buffer_size = ring_buffer_size(b);
+	if (buffer_size > capacity) {
+		wl_abort("ring_buffer_put: ring buffer corrupt: "
+		         "%zu - %zu > %zu\n", b->head, b->tail, capacity);
+	}
 
 	if (count == 0)
 		return 0;
 
+	if (capacity - buffer_size < count) {
+		wl_abort("ring_buffer_put: attempt to overfill buffer: "
+		         "%zu - %zu < %zu\n", capacity, buffer_size, count);
+	}
+
 	head = ring_buffer_mask(b, b->head);
-	if (head + count <= ring_buffer_capacity(b)) {
+	size = capacity - head;
+	if (count <= size) {
+		/* Enough space after head to fulfill request */
 		memcpy(b->data + head, data, count);
 	} else {
-		size = ring_buffer_capacity(b) - head;
+		/* Need to wrap around */
 		memcpy(b->data + head, data, size);
 		memcpy(b->data, (const char *) data + size, count - size);
 	}
@@ -113,78 +139,160 @@ ring_buffer_put(struct wl_ring_buffer *b, const void *data, size_t count)
 	return 0;
 }
 
+/* Precondition: the buffer is not full */
 static void
 ring_buffer_put_iov(struct wl_ring_buffer *b, struct iovec *iov, int *count)
 {
-	size_t head, tail;
+	size_t head, tail, size, capacity;
+
+	if (b->head < b->tail) {
+		wl_abort("ring_buffer_put_iov: ring buffer corrupt, %zu < %zu\n",
+		         b->head, b->tail);
+	}
+
+	size = ring_buffer_size(b);
+	capacity = ring_buffer_capacity(b);
+	if (size >= capacity) {
+		wl_abort("ring_buffer_put_iov: ring buffer full or corrupt: "
+		         "%zu - %zu >= %zu\n", b->head, b->tail, capacity);
+	}
 
 	head = ring_buffer_mask(b, b->head);
 	tail = ring_buffer_mask(b, b->tail);
 	if (head < tail) {
+		/* Buffer is like this:
+		 *         head              tail
+		 *           |                 |
+		 * +---------+-----------------+---------+
+		 * | VALID   | INVALID         | VALID   |
+		 * +---------+-----------------+---------+
+		 */
 		iov[0].iov_base = b->data + head;
 		iov[0].iov_len = tail - head;
 		*count = 1;
 	} else if (tail == 0) {
+		/* Buffer is like this:
+		 * tail                       head
+		 * |                           |
+		 * +---------------------------+---------+
+		 * | VALID                     | INVALID |
+		 * +---------------------------+---------+
+		 */
 		iov[0].iov_base = b->data + head;
-		iov[0].iov_len = ring_buffer_capacity(b) - head;
+		iov[0].iov_len = capacity - head;
 		*count = 1;
 	} else {
+		/* Buffer is like this:
+		 *         tail              head
+		 *           |                 |
+		 * +---------------------------+---------+
+		 * | INVALID | VALID           | INVALID |
+		 * +---------------------------+---------+
+		 */
 		iov[0].iov_base = b->data + head;
-		iov[0].iov_len = ring_buffer_capacity(b) - head;
+		iov[0].iov_len = capacity - head;
 		iov[1].iov_base = b->data;
 		iov[1].iov_len = tail;
 		*count = 2;
 	}
 }
 
+/* Precondition: the buffer is not empty */
 static void
 ring_buffer_get_iov(struct wl_ring_buffer *b, struct iovec *iov, int *count)
 {
-	size_t head, tail;
+	size_t head, tail, capacity;
+
+	if (b->head <= b->tail) {
+		wl_abort("ring_buffer_get_iov(): empty or corrupt buffer: %zu <= %zu\n",
+		         b->head, b->tail);
+	}
+
+	capacity = ring_buffer_capacity(b);
+	if (ring_buffer_size(b) > capacity) {
+		wl_abort("ring_buffer_put_iov: ring buffer corrupt: "
+		         "%zu - %zu > %zu\n", b->head, b->tail, capacity);
+	}
 
 	head = ring_buffer_mask(b, b->head);
 	tail = ring_buffer_mask(b, b->tail);
 	if (tail < head) {
+		/* Buffer is like this:
+		 *         tail              head
+		 *           |                 |
+		 * +---------+-----------------+---------+
+		 * | INVALID |     VALID       | INVALID |
+		 * +---------+-----------------+---------+
+		 */
 		iov[0].iov_base = b->data + tail;
 		iov[0].iov_len = head - tail;
 		*count = 1;
 	} else if (head == 0) {
+		/* Buffer is like this:
+		 * head                      tail
+		 * |                           |
+		 * +---------------------------+---------+
+		 * |             INVALID       | VALID   |
+		 * +---------------------------+---------+
+		 */
 		iov[0].iov_base = b->data + tail;
-		iov[0].iov_len = ring_buffer_capacity(b) - tail;
+		iov[0].iov_len = capacity - tail;
 		*count = 1;
 	} else {
+		/* Buffer is like this:
+		 *       head                tail
+		 *         |                   |
+		 * +-------+-------------------+---------+
+		 * | VALID |      INVALID      | VALID   |
+		 * +---------------------------+---------+
+		 */
 		iov[0].iov_base = b->data + tail;
-		iov[0].iov_len = ring_buffer_capacity(b) - tail;
+		iov[0].iov_len = capacity - tail;
 		iov[1].iov_base = b->data;
 		iov[1].iov_len = head;
 		*count = 2;
 	}
 }
 
+/* Precondition: the data will not underflow the buffer */
 static void
 ring_buffer_copy(struct wl_ring_buffer *b, void *data, size_t count)
 {
-	size_t tail, size;
+	size_t tail, size, buffer_size, capacity;
+
+	if (b->head < b->tail) {
+		wl_abort("ring_buffer_copy(): ring buffer corrupt, %zu < %zu\n",
+		         b->head, b->tail);
+	}
+
+	buffer_size = ring_buffer_size(b);
+	capacity = ring_buffer_capacity(b);
+	if (buffer_size > capacity) {
+		wl_abort("ring_buffer_copy(): ring buffer corrupt: "
+		         "%zu - %zu > %zu\n", b->head, b->tail, capacity);
+	}
 
 	if (count == 0)
 		return;
 
+	if (buffer_size < count) {
+		wl_abort("ring_buffer_copy(): attempt to copy %zu bytes "
+		         "but buffer has %zu bytes\n",
+		         count, buffer_size);
+	}
+
 	tail = ring_buffer_mask(b, b->tail);
-	if (tail + count <= ring_buffer_capacity(b)) {
+	size = capacity - tail;
+	if (count <= size) {
+		/* Enough data after the tail to fulfill the request */
 		memcpy(data, b->data + tail, count);
 	} else {
-		size = ring_buffer_capacity(b) - tail;
+		/* Must wrap buffer around */
 		memcpy(data, b->data + tail, size);
 		memcpy((char *) data + size, b->data, count - size);
 	}
 }
 
-static size_t
-ring_buffer_size(struct wl_ring_buffer *b)
-{
-	return b->head - b->tail;
-}
-
 static char *
 ring_buffer_tail(const struct wl_ring_buffer *b)
 {
@@ -413,7 +521,7 @@ decode_cmsg(struct wl_ring_buffer *buffer, struct msghdr *msg)
 {
 	struct cmsghdr *cmsg;
 	size_t size, i;
-	int overflow = 0;
+	bool overflow = false;
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg != NULL;
 	     cmsg = CMSG_NXTHDR(msg, cmsg)) {
@@ -424,7 +532,7 @@ decode_cmsg(struct wl_ring_buffer *buffer, struct msghdr *msg)
 		size = cmsg->cmsg_len - CMSG_LEN(0);
 
 		if (ring_buffer_ensure_space(buffer, size) < 0 || overflow) {
-			overflow = 1;
+			overflow = true;
 			size /= sizeof(int32_t);
 			for (i = 0; i < size; i++)
 				close(((int*)CMSG_DATA(cmsg))[i]);
@@ -456,6 +564,9 @@ wl_connection_flush(struct wl_connection *connection)
 
 	tail = connection->out.tail;
 	while (ring_buffer_size(&connection->out) > 0) {
+		/* Ring buffer is not empty, so this is safe. */
+		ring_buffer_get_iov(&connection->out, iov, &count);
+
 		build_cmsg(&connection->fds_out, cmsg, &clen);
 
 		if (clen >= CLEN) {
@@ -530,6 +641,7 @@ wl_connection_read(struct wl_connection *connection)
 		if (ring_buffer_ensure_space(&connection->in, 1) < 0)
 			return -1;
 
+		/* Ring buffer is not full, so this is safe. */
 		ring_buffer_put_iov(&connection->in, iov, &count);
 
 		msg.msg_name = NULL;
@@ -683,6 +795,7 @@ wl_message_get_since(const struct wl_message *message)
 {
 	int since;
 
+	/* This is trusted input */
 	since = atoi(message->signature);
 
 	if (since == 0)
@@ -950,14 +1063,14 @@ wl_connection_demarshal(struct wl_connection *connection,
 		case WL_ARG_STRING:
 			length = *p++;
 
-			if (length == 0 && !arg.nullable) {
-				wl_log("NULL string received on non-nullable "
-				       "type, message %s(%s)\n", message->name,
-				       message->signature);
-				errno = EINVAL;
-				goto err;
-			}
 			if (length == 0) {
+				if (!arg.nullable) {
+					wl_log("NULL string received on non-nullable "
+							"type, message %s(%s)\n", message->name,
+							message->signature);
+					errno = EINVAL;
+					goto err;
+				}
 				closure->args[i].s = NULL;
 				break;
 			}
@@ -1060,7 +1173,10 @@ wl_connection_demarshal(struct wl_connection *connection,
 				goto err;
 			}
 
+			/* This ring buffer will always have a multiple of sizeof(int)
+			 * bytes in it. */
 			ring_buffer_copy(&connection->fds_in, &fd, sizeof fd);
+			/* This can wrap but that is okay. */
 			connection->fds_in.tail += sizeof fd;
 			closure->args[i].h = fd;
 			break;

src/wayland-server.c

@@ -740,8 +740,7 @@ wl_client_post_implementation_error(struct wl_client *client,
 WL_EXPORT void
 wl_resource_post_no_memory(struct wl_resource *resource)
 {
-	wl_resource_post_error(resource->client->display_resource,
-			       WL_DISPLAY_ERROR_NO_MEMORY, "no memory");
+	wl_client_post_no_memory(resource->client);
 }
 
 /** Detect if a wl_resource uses the deprecated public definition.