improve request header handling

We should require a non-empty payload for every command type except
VFIO_USER_DEVICE_RESET.

We should also reply to the caller with such failures.

Add some testing for is_valid_header(), and move the fd handling test over to it
too.

Signed-off-by: John Levon <john.levon@nutanix.com>
Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

5 files changed, 132 insertions, 128 deletions

diff --git a/test/mocks.c b/test/mocks.c
index 5a2b55f..4f202d5 100644
--- a/test/mocks.c
+++ b/test/mocks.c
@@ -62,12 +62,9 @@ static struct function funcs[] = {
     { .name = "dma_controller_add_region" },
     { .name = "dma_controller_remove_region" },
     { .name = "dma_controller_unmap_region" },
-    { .name = "exec_command" },
-    { .name = "get_request_header" },
     { .name = "handle_dirty_pages" },
     { .name = "process_request" },
     { .name = "should_exec_command" },
-    { .name = "tran_sock_send_iovec" },
     { .name = "migration_region_access_registers" },
     { .name = "handle_dirty_pages_get" },
     { .name = "handle_device_state" },
@@ -164,24 +161,6 @@ dma_controller_unmap_region(dma_controller_t *dma,
     check_expected(region);
 }
 
-int
-tran_sock_send_iovec(int sock, uint16_t msg_id, bool is_reply,
-                     enum vfio_user_command cmd,
-                     struct iovec *iovecs, size_t nr_iovecs,
-                     int *fds, int count, int err)
-{
-    check_expected(sock);
-    check_expected(msg_id);
-    check_expected(is_reply);
-    check_expected(cmd);
-    check_expected(iovecs);
-    check_expected(nr_iovecs);
-    check_expected(fds);
-    check_expected(count);
-    check_expected(err);
-    return mock();
-}
-
 bool
 device_is_stopped(struct migration *migration)
 {
@@ -193,26 +172,6 @@ device_is_stopped(struct migration *migration)
 }
 
 int
-get_request_header(vfu_ctx_t *vfu_ctx, vfu_msg_t **msgp)
-{
-    check_expected(vfu_ctx);
-    check_expected(msgp);
-    return mock();
-}
-
-int
-exec_command(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
-{
-    if (!is_patched("exec_command")) {
-        return __real_exec_command(vfu_ctx, msg);
-    }
-    check_expected(vfu_ctx);
-    check_expected(msg);
-    errno = mock();
-    return mock();
-}
-
-int
 process_request(vfu_ctx_t *vfu_ctx)
 {
 
diff --git a/test/py/libvfio_user.py b/test/py/libvfio_user.py
index a73c228..31e9767 100644
--- a/test/py/libvfio_user.py
+++ b/test/py/libvfio_user.py
@@ -442,7 +442,7 @@ def vfio_user_header(cmd, size, no_reply=False, error=False, error_no=0):
     global msg_id
 
     buf = struct.pack("HHIII", msg_id, cmd, SIZEOF_VFIO_USER_HEADER + size,
-                      0, error_no)
+                      VFIO_USER_F_TYPE_COMMAND, error_no)
 
     msg_id += 1
 
diff --git a/test/py/test_device_set_irqs.py b/test/py/test_device_set_irqs.py
index af3a03e..9e54381 100644
--- a/test/py/test_device_set_irqs.py
+++ b/test/py/test_device_set_irqs.py
@@ -59,6 +59,12 @@ def test_device_set_irqs_setup():
 
     sock = connect_client(ctx)
 
+def test_device_set_irqs_no_irq_set():
+    hdr = vfio_user_header(VFIO_USER_DEVICE_SET_IRQS, size=0)
+    sock.send(hdr)
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
 def test_device_set_irqs_short_write():
     payload = struct.pack("II", 0, 0)
 
diff --git a/test/py/test_request_errors.py b/test/py/test_request_errors.py
new file mode 100644
index 0000000..fe6f29e
--- /dev/null
+++ b/test/py/test_request_errors.py
@@ -0,0 +1,124 @@
+#
+# Copyright (c) 2021 Nutanix Inc. All rights reserved.
+#
+# Authors: John Levon <john.levon@nutanix.com>
+#
+#  Redistribution and use in source and binary forms, with or without
+#  modification, are permitted provided that the following conditions are met:
+#      * Redistributions of source code must retain the above copyright
+#        notice, this list of conditions and the following disclaimer.
+#      * Redistributions in binary form must reproduce the above copyright
+#        notice, this list of conditions and the following disclaimer in the
+#        documentation and/or other materials provided with the distribution.
+#      * Neither the name of Nutanix nor the names of its contributors may be
+#        used to endorse or promote products derived from this software without
+#        specific prior written permission.
+#
+#  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+#  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+#  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+#  ARE DISCLAIMED. IN NO EVENT SHALL <COPYRIGHT HOLDER> BE LIABLE FOR ANY
+#  DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+#  (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+#  SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+#  CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+#  LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+#  OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+#  DAMAGE.
+#
+
+from libvfio_user import *
+import ctypes as c
+import errno
+import os
+import sys
+
+ctx = None
+sock = None
+
+argsz = len(vfio_irq_set())
+
+def test_request_errors_setup():
+    global ctx, sock
+
+    ctx = vfu_create_ctx(flags=LIBVFIO_USER_FLAG_ATTACH_NB)
+    assert ctx != None
+
+    ret = vfu_pci_init(ctx)
+    assert ret == 0
+
+    ret = vfu_setup_device_nr_irqs(ctx, VFU_DEV_MSIX_IRQ, 2048)
+    assert ret == 0
+
+    ret = vfu_realize_ctx(ctx)
+    assert ret == 0
+
+    sock = connect_client(ctx)
+
+def test_too_small():
+    # struct vfio_user_header
+    hdr = struct.pack("HHIII", 0xbad1, VFIO_USER_DEVICE_SET_IRQS,
+                      SIZEOF_VFIO_USER_HEADER - 1, VFIO_USER_F_TYPE_COMMAND, 0)
+
+    sock.send(hdr)
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
+def test_too_large():
+    # struct vfio_user_header
+    hdr = struct.pack("HHIII", 0xbad1, VFIO_USER_DEVICE_SET_IRQS,
+                      SERVER_MAX_MSG_SIZE + 1, VFIO_USER_F_TYPE_COMMAND, 0)
+
+    sock.send(hdr)
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
+def test_unsolicited_reply():
+    # struct vfio_user_header
+    hdr = struct.pack("HHIII", 0xbad2, VFIO_USER_DEVICE_SET_IRQS,
+                      SIZEOF_VFIO_USER_HEADER, VFIO_USER_F_TYPE_REPLY, 0)
+
+    sock.send(hdr)
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
+def test_bad_command():
+    hdr = vfio_user_header(VFIO_USER_MAX, size=1)
+
+    sock.send(hdr + b'\0')
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
+def test_no_payload():
+    hdr = vfio_user_header(VFIO_USER_DEVICE_SET_IRQS, size=0)
+    sock.send(hdr)
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
+def test_bad_request_closes_fds():
+    payload = vfio_irq_set(argsz=argsz, flags=VFIO_IRQ_SET_ACTION_TRIGGER |
+                           VFIO_IRQ_SET_DATA_BOOL, index=VFU_DEV_MSIX_IRQ,
+                           start=0, count=1)
+
+    fd1 = eventfd()
+    fd2 = eventfd()
+
+    hdr = vfio_user_header(VFIO_USER_DEVICE_SET_IRQS, size=len(payload))
+    sock.sendmsg([hdr + payload], [(socket.SOL_SOCKET, socket.SCM_RIGHTS,
+                 struct.pack("II", fd1, fd2))])
+    vfu_run_ctx(ctx)
+    get_reply(sock, expect=errno.EINVAL)
+
+    #
+    # It's a little cheesy, but this is just ensuring no fd's remain open past
+    # the one we just allocated; i.e. free_msg() freed the fds it got.
+    #
+    test_fd = eventfd()
+    assert test_fd == fd2 + 1
+    os.close(test_fd)
+
+    os.close(fd1)
+    os.close(fd2)
+
+def test_request_errors_cleanup():
+    vfu_destroy_ctx(ctx)
diff --git a/test/unit-tests.c b/test/unit-tests.c
index e712867..4f39228 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -69,6 +69,7 @@ static vfu_msg_t *
 mkmsg(enum vfio_user_command cmd, void *data, size_t size)
 {
     msg.hdr.cmd = cmd;
+    msg.hdr.msg_size = size;
     msg.in_data = data;
     msg.in_size = size;
 
@@ -681,91 +682,6 @@ test_should_exec_command(UNUSED void **state)
     assert_true(should_exec_command(&vfu_ctx, 0xbeef));
 }
 
-static int
-check_request_header_msg(const LargestIntegralType value,
-                         const LargestIntegralType cvalue UNUSED)
-{
-    vfu_msg_t **msgp = (vfu_msg_t **)value;
-
-    *msgp = malloc(sizeof(msg));
-
-    assert_non_null(*msgp);
-
-    memcpy(*msgp, &msg, sizeof(msg));
-
-    return 1;
-}
-
-static int
-check_exec_command_msg(const LargestIntegralType value,
-                       const LargestIntegralType cvalue UNUSED)
-{
-    vfu_msg_t *cmsg = (vfu_msg_t *)value;
-
-    int ret = cmsg->nr_in_fds == ARRAY_SIZE(fds) &&
-              cmsg->in_fds[0] == fds[0] &&
-              cmsg->in_fds[1] == fds[1] &&
-              cmsg->in_data == NULL &&
-              cmsg->in_size == 0 &&
-              memcmp(&cmsg->hdr, &msg.hdr, sizeof (msg.hdr)) == 0;
-
-    consume_fd(cmsg->in_fds, cmsg->nr_in_fds, 0);
-
-    return ret;
-}
-
-/*
- * Tests that if if exec_command fails then process_request() frees passed file
- * descriptors.
- */
-static void
-test_process_request_free_passed_fds(void **state UNUSED)
-{
-    tran_sock_t ts = { .fd = 23, .conn_fd = 24 };
-
-    mkmsg(VFIO_USER_DMA_MAP, NULL, 0);
-
-    fds[0] = 0xab;
-    fds[1] = 0xcd;
-    msg.nr_in_fds = 2;
-    msg.in_fds = malloc(sizeof(int) * msg.nr_in_fds);
-    assert_non_null(msg.in_fds);
-    msg.in_fds[0] = fds[0];
-    msg.in_fds[1] = fds[1];
-
-    vfu_ctx.tran = &tran_sock_ops;
-    vfu_ctx.tran_data = &ts;
-
-    patch("get_request_header");
-    expect_value(get_request_header, vfu_ctx, &vfu_ctx);
-    expect_check(get_request_header, msgp, check_request_header_msg, NULL);
-    will_return(get_request_header, 0);
-
-    patch("exec_command");
-    expect_value(exec_command, vfu_ctx, &vfu_ctx);
-    expect_check(exec_command, msg, check_exec_command_msg, NULL);
-    will_return(exec_command, -1);
-    will_return(exec_command, EREMOTEIO);
-
-    patch("close");
-    expect_value(close, fd, fds[1]);
-    will_return(close, 0);
-
-    patch("tran_sock_send_iovec");
-    expect_value(tran_sock_send_iovec, sock, ts.conn_fd);
-    expect_any(tran_sock_send_iovec, msg_id);
-    expect_value(tran_sock_send_iovec, is_reply, true);
-    expect_any(tran_sock_send_iovec, cmd);
-    expect_any(tran_sock_send_iovec, iovecs);
-    expect_any(tran_sock_send_iovec, nr_iovecs);
-    expect_any(tran_sock_send_iovec, fds);
-    expect_any(tran_sock_send_iovec, count);
-    expect_any(tran_sock_send_iovec, err);
-    will_return(tran_sock_send_iovec, 0);
-
-    assert_int_equal(0, process_request(&vfu_ctx));
-}
-
 static void
 test_dma_controller_dirty_page_get(void **state UNUSED)
 {
@@ -818,7 +734,6 @@ main(void)
         cmocka_unit_test_setup(test_device_is_stopped_and_copying, setup),
         cmocka_unit_test_setup(test_cmd_allowed_when_stopped_and_copying, setup),
         cmocka_unit_test_setup(test_should_exec_command, setup),
-        cmocka_unit_test_setup(test_process_request_free_passed_fds, setup),
         cmocka_unit_test_setup(test_dma_controller_dirty_page_get, setup),
     };