On Thu, Apr 02, 2026 at 03:28:25PM +0200, Stefano Garzarella wrote:
On Thu, Apr 02, 2026 at 10:18:02AM +0200, Luigi Leonardi wrote:
Add a test that verifies MSG_PEEK works correctly after a partial
recv().
This is to test a bug that was present in the
`virtio_transport_stream_do_peek()`
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit
description?)
#11: This is to test a bug that was present in the
`virtio_transport_stream_do_peek()`
oops, thanks :)
when computing the number of bytes to copy: After a partial read, the
peek function didn't take into consideration the number of bytes that
were already read. So peeking the whole buffer would cause a out-of-bounds read,
that resulted in a -EFAULT.
This test does exactly this: do a partial recv on a buffer, then try to
peek the whole buffer content.
Signed-off-by: Luigi Leonardi <[email protected]>
---
tools/testing/vsock/vsock_test.c | 64 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index
5bd20ccd9335caafe68e8b7a5d02a4deb3d2deec..308f9f8f30d22bec5aaa282356e400d8438fe321
100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -346,6 +346,65 @@ static void test_stream_msg_peek_server(const struct
test_opts *opts)
return test_msg_peek_server(opts, false);
}
+#define PEEK_AFTER_RECV_LEN 100
Why 100 ?
Better to use a power of 2 IMO like we do in all other cases IIRC.
Right, I'll reuse `MSG_PEEK_BUF_LEN`.
+
+static void test_stream_peek_after_recv_client(const struct test_opts *opts)
+{
+ unsigned char buf[PEEK_AFTER_RECV_LEN];
+ int fd;
+ int i;
nit: int fd, i;
+
+ fd = vsock_stream_connect(opts->peer_cid, opts->peer_port);
+ if (fd < 0) {
+ perror("connect");
+ exit(EXIT_FAILURE);
+ }
+
+ for (i = 0; i < sizeof(buf); i++)
+ buf[i] = (unsigned char)i;
Why setting the payload in this way ? Can we just do a memset() ?
Good point.
+
+ control_expectln("SRVREADY");
Why we need this barrier ?
leftover from development, will remove.
+
+ send_buf(fd, buf, sizeof(buf), 0, sizeof(buf));
+
+ close(fd);
+}
+
+static void test_stream_peek_after_recv_server(const struct test_opts *opts)
+{
+ unsigned char buf[PEEK_AFTER_RECV_LEN];
+ int half = PEEK_AFTER_RECV_LEN / 2;
+ ssize_t ret;
+ int fd;
+
+ fd = vsock_stream_accept(VMADDR_CID_ANY, opts->peer_port, NULL);
+ if (fd < 0) {
+ perror("accept");
+ exit(EXIT_FAILURE);
+ }
+
+ control_writeln("SRVREADY");
+
+ /* Partial recv to advance offset within the skb */
+ recv_buf(fd, buf, half, 0, half);
Why reading half of the size ?
IMO is better to read just 1 byte, since it is almost certain that an
skb does not have a 1-byte payload.
will do
+
+ /* Try to peek more than what remains: should return only 'half'
How we are sure that the sender sent all the bytes ?
+ * bytes. Note: we can't use recv_buf() because it loops until
+ * all requested bytes are returned.
Why this is a problem ? (an useful comment should explain the reason)
Some changes are required to `recv_buf`, I have a working v2 version
that uses that. Thanks for the hint.
+ */
+ ret = recv(fd, buf, sizeof(buf), MSG_PEEK);
+ if (ret < 0) {
Should we handle EINTR like we do in recv_buf() ?
But I still don't understand why we can't use it directly.
Thanks,
Stefano
+ perror("recv");
+ exit(EXIT_FAILURE);
+ } else if (ret != half) {
+ fprintf(stderr, "MSG_PEEK after partial recv returned %d (expected
%d)\n",
+ ret, half);
+ exit(EXIT_FAILURE);
+ }
+
+ close(fd);
+}
+
#define SOCK_BUF_SIZE (2 * 1024 * 1024)
#define SOCK_BUF_SIZE_SMALL (64 * 1024)
#define MAX_MSG_PAGES 4
@@ -2520,6 +2579,11 @@ static struct test_case test_cases[] = {
.run_client = test_stream_tx_credit_bounds_client,
.run_server = test_stream_tx_credit_bounds_server,
},
+ {
+ .name = "SOCK_STREAM MSG_PEEK after partial recv",
+ .run_client = test_stream_peek_after_recv_client,
+ .run_server = test_stream_peek_after_recv_server,
+ },
{},
};
--
2.53.0
