don't leak passed file descriptors on failure

Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>

3 files changed, 239 insertions, 6 deletions

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 822e948..23fdd36 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -46,6 +46,12 @@ target_link_libraries(unit-tests PUBLIC
                       "-Wl,--wrap=_dma_controller_do_remove_region")
 target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=dma_controller_add_region")
 target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=dma_map_region")
+target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=device_is_stopped")
+target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=get_next_command")
+target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=exec_command")
+target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=close")
+target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=vfu_send_iovec")
+target_link_libraries(unit-tests PUBLIC "-Wl,--wrap=free")
 
 enable_testing()
 add_test(NAME unit-tests COMMAND unit-tests)
diff --git a/test/mocks.c b/test/mocks.c
index 7852c1d..ac0a986 100644
--- a/test/mocks.c
+++ b/test/mocks.c
@@ -36,6 +36,7 @@
 
 #include "mocks.h"
 #include "dma.h"
+#include "migration.h"
 
 struct function
 {
@@ -56,7 +57,7 @@ __wrap_dma_controller_add_region(dma_controller_t *dma, dma_addr_t dma_addr,
     check_expected(size);
     check_expected(fd);
     check_expected(offset);
-    return 0;
+    return mock();
 }
 
 void *
@@ -67,7 +68,7 @@ __wrap_dma_map_region(dma_memory_region_t *region, int prot, size_t offset,
     check_expected(prot);
     check_expected(offset);
     check_expected(len);
-    return 0;
+    return mock_ptr_type(void*);
 }
 
 void
@@ -78,11 +79,85 @@ __wrap__dma_controller_do_remove_region(dma_controller_t *dma,
     check_expected(region);
 }
 
+bool
+__wrap_device_is_stopped(struct migration *migr)
+{
+    check_expected(migr);
+    return mock();
+}
+
+int
+__wrap_get_next_command(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr,
+                        int *fds, size_t *nr_fds)
+{
+    check_expected(vfu_ctx);
+    check_expected(hdr);
+    check_expected(fds);
+    check_expected(nr_fds);
+    return mock();
+}
+
+int
+__wrap_exec_command(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr,
+                    size_t size, int *fds, size_t *nr_fds,
+                    struct iovec *_iovecs, struct iovec **iovecs,
+                    size_t *nr_iovecs, bool *free_iovec_data)
+{
+    check_expected(vfu_ctx);
+    check_expected(hdr);
+    check_expected(size);
+    check_expected(fds);
+    check_expected(nr_fds);
+    check_expected(_iovecs);
+    check_expected(iovecs);
+    check_expected(nr_iovecs);
+    check_expected(free_iovec_data);
+    return mock();
+}
+
+int
+__wrap_close(int fd)
+{
+    check_expected(fd);
+    return mock();
+}
+
+int
+__wrap_vfu_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();
+}
+
+void
+__wrap_free(void *ptr)
+{
+    assert(false);
+    check_expected(ptr);
+}
+
 /* FIXME should be something faster than unsorted array, look at tsearch(3). */
 static struct function funcs[] = {
     {.addr = &__wrap_dma_controller_add_region},
     {.addr = &__wrap_dma_map_region},
     {.addr = &__wrap__dma_controller_do_remove_region},
+    {.addr = &__wrap_device_is_stopped},
+    {.addr = &__wrap_get_next_command},
+    {.addr = &__wrap_exec_command},
+    {.addr = &__wrap_close},
+    {.addr = &__wrap_vfu_send_iovec},
+    {.addr = &__wrap_free}
 };
 
 static struct function*
diff --git a/test/unit-tests.c b/test/unit-tests.c
index 19375b8..c09c8a9 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -37,10 +37,13 @@
 #include <stdio.h>
 #include <assert.h>
 #include <alloca.h>
+#include <string.h>
 
 #include "dma.h"
 #include "libvfio-user.h"
 #include "private.h"
+#include "migration.h"
+#include "tran_sock.h"
 
 static void
 test_dma_map_without_dma(void **state __attribute__((unused)))
@@ -51,8 +54,9 @@ test_dma_map_without_dma(void **state __attribute__((unused)))
         .flags = VFIO_USER_F_DMA_REGION_MAPPABLE
     };
     int fd;
+    size_t nr_fds = 1;
 
-    assert_int_equal(0, handle_dma_map_or_unmap(&vfu_ctx, size, true, &fd, 1, &dma_region));
+    assert_int_equal(0, handle_dma_map_or_unmap(&vfu_ctx, size, true, &fd, &nr_fds, &dma_region));
 }
 
 static void
@@ -65,7 +69,9 @@ test_dma_map_mappable_without_fd(void **state __attribute__((unused)))
         .flags = VFIO_USER_F_DMA_REGION_MAPPABLE
     };
     int fd;
-    assert_int_equal(-EINVAL, handle_dma_map_or_unmap(&vfu_ctx, size, true, &fd, 0, &dma_region));
+    size_t nr_fds = 0;
+
+    assert_int_equal(-EINVAL, handle_dma_map_or_unmap(&vfu_ctx, size, true, &fd, &nr_fds, &dma_region));
 }
 
 static void
@@ -82,14 +88,16 @@ test_dma_map_without_fd(void **state __attribute__((unused)))
         .offset = 0x8badf00d
     };
     int fd;
+    size_t nr_fds = 0;
 
     patch(dma_controller_add_region);
+    will_return(__wrap_dma_controller_add_region, 0);
     expect_value(__wrap_dma_controller_add_region, dma, vfu_ctx.dma);
     expect_value(__wrap_dma_controller_add_region, dma_addr, r.addr);
     expect_value(__wrap_dma_controller_add_region, size, r.size);
     expect_value(__wrap_dma_controller_add_region, fd, -1);
     expect_value(__wrap_dma_controller_add_region, offset, r.offset);
-    assert_int_equal(0, handle_dma_map_or_unmap(&vfu_ctx, size, true, &fd, 0, &r));
+    assert_int_equal(0, handle_dma_map_or_unmap(&vfu_ctx, size, true, &fd, &nr_fds, &r));
 }
 
 /*
@@ -116,8 +124,11 @@ test_dma_add_regions_mixed(void **state __attribute__((unused)))
         }
     };
     int fd = 0x8badf00d;
+    size_t nr_fds = 1;
 
     patch(dma_controller_add_region);
+    will_return(__wrap_dma_controller_add_region, 0);
+    will_return(__wrap_dma_controller_add_region, 0);
     expect_value(__wrap_dma_controller_add_region, dma, vfu_ctx.dma);
     expect_value(__wrap_dma_controller_add_region, dma_addr, r[0].addr);
     expect_value(__wrap_dma_controller_add_region, size, r[0].size);
@@ -129,9 +140,73 @@ test_dma_add_regions_mixed(void **state __attribute__((unused)))
     expect_value(__wrap_dma_controller_add_region, fd, fd);
     expect_value(__wrap_dma_controller_add_region, offset, r[1].offset);
 
-    assert_int_equal(0, handle_dma_map_or_unmap(&vfu_ctx, sizeof r, true, &fd, 1, r));
+    assert_int_equal(0, handle_dma_map_or_unmap(&vfu_ctx, sizeof r, true, &fd, &nr_fds, r));
 }
 
+/*
+ * Tests that handle_dma_map_or_unmap sets the correct number of file
+ * descriptors when failing halfway through.
+ */
+static void
+test_dma_add_regions_mixed_partial_failure(void **state __attribute__((unused)))
+{
+    dma_controller_t dma = { 0 };
+    vfu_ctx_t vfu_ctx = { .dma = &dma };
+    dma.vfu_ctx = &vfu_ctx;
+    struct vfio_user_dma_region r[3] = {
+        [0] = {
+            .addr = 0xdeadbeef,
+            .size = 0x1000,
+            .offset = 0
+        },
+        [1] = {
+            .addr = 0xcafebabe,
+            .size = 0x1000,
+            .offset = 0x1000,
+            .flags = VFIO_USER_F_DMA_REGION_MAPPABLE
+        },
+        [2] = {
+            .addr = 0xbabecafe,
+            .size = 0x1000,
+            .offset = 0x2000,
+            .flags = VFIO_USER_F_DMA_REGION_MAPPABLE
+        }
+    };
+    int fds[] = {0x8badf00d, 0xbad8f00d};
+    size_t nr_fds = 2;
+
+    patch(dma_controller_add_region);
+
+    /* 1st region */
+    expect_value(__wrap_dma_controller_add_region, dma, vfu_ctx.dma);
+    expect_value(__wrap_dma_controller_add_region, dma_addr, r[0].addr);
+    expect_value(__wrap_dma_controller_add_region, size, r[0].size);
+    expect_value(__wrap_dma_controller_add_region, fd, -1);
+    expect_value(__wrap_dma_controller_add_region, offset, r[0].offset);
+    will_return(__wrap_dma_controller_add_region, 0);
+
+    /* 2nd region */
+    expect_value(__wrap_dma_controller_add_region, dma, vfu_ctx.dma);
+    expect_value(__wrap_dma_controller_add_region, dma_addr, r[1].addr);
+    expect_value(__wrap_dma_controller_add_region, size, r[1].size);
+    expect_value(__wrap_dma_controller_add_region, fd, fds[0]);
+    expect_value(__wrap_dma_controller_add_region, offset, r[1].offset);
+    will_return(__wrap_dma_controller_add_region, 0);
+
+    /* 3rd region */
+    expect_value(__wrap_dma_controller_add_region, dma, vfu_ctx.dma);
+    expect_value(__wrap_dma_controller_add_region, dma_addr, r[2].addr);
+    expect_value(__wrap_dma_controller_add_region, size, r[2].size);
+    expect_value(__wrap_dma_controller_add_region, fd, fds[1]);
+    expect_value(__wrap_dma_controller_add_region, offset, r[2].offset);
+    will_return(__wrap_dma_controller_add_region, -0x1234);
+
+    assert_int_equal(-0x1234,
+                     handle_dma_map_or_unmap(&vfu_ctx,
+                                             ARRAY_SIZE(r) * sizeof(struct vfio_user_dma_region),
+                                             true, fds, &nr_fds, r));
+    assert_int_equal(1, nr_fds);
+}
 
 static void
 test_dma_controller_add_region_no_fd(void **state __attribute__((unused)))
@@ -178,6 +253,81 @@ test_dma_controller_remove_region_no_fd(void **state __attribute__((unused)))
 }
 
 /*
+ * Tests that if if exec_command fails then process_request frees passed file
+ * descriptors.
+ */
+static void
+test_process_command_free_passed_fds(void **state __attribute__((unused)))
+{
+    int fds[] = {0xab, 0xcd};
+    int set_fds(const long unsigned int value,
+                const long unsigned int data __attribute__((unused)))
+    {
+        assert(value != 0);
+        memcpy((int*)value, fds, ARRAY_SIZE(fds) * sizeof(int));
+        return 1;
+    }
+    int set_nr_fds(const long unsigned int value,
+                   const long unsigned int data)
+    {
+        int *nr_fds = (int*)value;
+        assert(nr_fds != NULL);
+        if ((void*)data == &get_next_command) {
+            *nr_fds = ARRAY_SIZE(fds);
+        } else if ((void*)data == &exec_command) {
+            *nr_fds = 1;
+        }
+        return 1;
+    }
+
+    vfu_ctx_t vfu_ctx = {
+        .conn_fd = 0xcafebabe,
+        .migration = (struct migration*)0x8badf00d
+    };
+
+    patch(device_is_stopped);
+    expect_value(__wrap_device_is_stopped, migr, vfu_ctx.migration);
+    will_return(__wrap_device_is_stopped, false);
+
+    patch(get_next_command);
+    expect_value(__wrap_get_next_command, vfu_ctx, &vfu_ctx);
+    expect_any(__wrap_get_next_command, hdr);
+    expect_check(__wrap_get_next_command, fds, &set_fds, &get_next_command);
+    expect_check(__wrap_get_next_command, nr_fds, &set_nr_fds, &get_next_command);
+    will_return(__wrap_get_next_command, 0x0000beef);
+
+    patch(exec_command);
+    expect_value(__wrap_exec_command, vfu_ctx, &vfu_ctx);
+    expect_any(__wrap_exec_command, hdr);
+    expect_value(__wrap_exec_command, size, 0x0000beef);
+    expect_any(__wrap_exec_command, fds);
+    expect_check(__wrap_exec_command, nr_fds, &set_nr_fds, &exec_command);
+    expect_any(__wrap_exec_command, _iovecs);
+    expect_any(__wrap_exec_command, iovecs);
+    expect_any(__wrap_exec_command, nr_iovecs);
+    expect_any(__wrap_exec_command, free_iovec_data);
+    will_return(__wrap_exec_command, -0x1234);
+
+    patch(close);
+    expect_value(__wrap_close, fd, 0xcd);
+    will_return(__wrap_close, 0);
+
+    patch(vfu_send_iovec);
+    expect_value(__wrap_vfu_send_iovec, sock, vfu_ctx.conn_fd);
+    expect_any(__wrap_vfu_send_iovec, msg_id);
+    expect_value(__wrap_vfu_send_iovec, is_reply, true);
+    expect_any(__wrap_vfu_send_iovec, cmd);
+    expect_any(__wrap_vfu_send_iovec, iovecs);
+    expect_any(__wrap_vfu_send_iovec, nr_iovecs);
+    expect_any(__wrap_vfu_send_iovec, fds);
+    expect_any(__wrap_vfu_send_iovec, count);
+    expect_any(__wrap_vfu_send_iovec, err);
+    will_return(__wrap_vfu_send_iovec, 0);
+
+    assert_int_equal(0, process_request(&vfu_ctx));
+}
+
+/*
  * FIXME we shouldn't have to specify a setup function explicitly for each unit
  * test, cmocka should provide that. E.g. cmocka_run_group_tests enables us to
  * run a function before/after ALL unit tests have finished, we can extend it
@@ -196,8 +346,10 @@ int main(void)
         cmocka_unit_test_setup(test_dma_map_mappable_without_fd, setup),
         cmocka_unit_test_setup(test_dma_map_without_fd, setup),
         cmocka_unit_test_setup(test_dma_add_regions_mixed, setup),
+        cmocka_unit_test_setup(test_dma_add_regions_mixed_partial_failure, setup),
         cmocka_unit_test_setup(test_dma_controller_add_region_no_fd, setup),
         cmocka_unit_test_setup(test_dma_controller_remove_region_no_fd, setup),
+        cmocka_unit_test_setup(test_process_command_free_passed_fds, setup),
     };
 
     return cmocka_run_group_tests(tests, NULL, NULL);