Introduce close_safely helper function (nutanix#763)

The helper function centralizes some extra checks and diligence desired by many/most current code paths but currently inconsistently applied. This includes bypassing the close call when the file descriptor is -1 already, resetting the file descriptor variable to -1 after closing, and preserving errno. All calls to close are replaced by close_safely. Some warning log output is lost over this, but it doesn't seem like this was very useful anyways given that Linux always closes the file descriptor anyways. Signed-off-by: Mattias Nissler <[email protected]>
vmuxIO · Aug 15, 2023 · e8c37f8 · e8c37f8
1 parent 1cca91a
commit e8c37f8
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 63 deletions.
diff --git a/lib/common.h b/lib/common.h
@@ -37,8 +37,10 @@
 #ifndef LIB_VFIO_USER_COMMON_H
 #define LIB_VFIO_USER_COMMON_H
 
+#include <errno.h>
 #include <limits.h>
 #include <stdint.h>
+#include <unistd.h>
 
 #define UNUSED __attribute__((unused))
 #define EXPORT __attribute__((visibility("default")))
@@ -79,6 +81,32 @@ _get_bitmap_size(size_t size, size_t pgsize)
     return ROUND_UP(nr_pages, sizeof(uint64_t) * CHAR_BIT) / CHAR_BIT;
 }
 
+/*
+ * Closes the given file descriptor, and resets the value to -1. Preserves
+ * errno. Skips closing if *fd is -1.
+ */
+static inline void
+close_safely(int *fd)
+{
+    int saved_errno = errno;
+    if (fd != NULL && *fd != -1) {
+        /*
+         * POSIX says that close may hit EINTR and leave the file descriptor in
+         * undefined state. But retrying on EINTR is incorrect, since a
+         * different thread might have re-opened a file on the same descriptor
+         * if close actually did free the descriptor. In practice, Linux always
+         * closes the file descriptor and POSIX has decided to align semantics
+         * with Linux. Thus, calling close once and ignoring the error is the
+         * most appropriate course of action.
+         *
+         * See also https://www.austingroupbugs.net/view.php?id=529
+         */
+        (void) close(*fd);
+        *fd = -1;
+    }
+    errno = saved_errno;
+}
+
 #ifdef UNIT_TEST
 
 #define MOCK_DEFINE(f) \

diff --git a/lib/dma.c b/lib/dma.c
@@ -121,10 +121,7 @@ MOCK_DEFINE(dma_controller_unmap_region)(dma_controller_t *dma,
 
     assert(region->fd != -1);
 
-    if (close(region->fd) == -1) {
-        vfu_log(dma->vfu_ctx, LOG_WARNING, "failed to close fd %d: %m",
-                region->fd);
-    }
+    close_safely(&region->fd);
 }
 
 static void
@@ -402,10 +399,7 @@ MOCK_DEFINE(dma_controller_add_region)(dma_controller_t *dma,
             vfu_log(dma->vfu_ctx, LOG_ERR,
                    "failed to memory map DMA region %s: %m", rstr);
 
-            if (close(region->fd) == -1) {
-                vfu_log(dma->vfu_ctx, LOG_WARNING,
-                        "failed to close fd %d: %m", region->fd);
-            }
+            close_safely(&region->fd);
             free(region->dirty_bitmap);
             return ERROR_INT(ret);
         }

diff --git a/lib/irq.c b/lib/irq.c
@@ -122,14 +122,7 @@ irqs_disable(vfu_ctx_t *vfu_ctx, uint32_t index, uint32_t start, uint32_t count)
     }
 
     for (i = start; i < count; i++) {
-        if (efds[i] >= 0) {
-            if (close(efds[i]) == -1) {
-                vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m",
-                        efds[i]);
-            }
-
-            efds[i] = -1;
-        }
+        close_safely(&efds[i]);
     }
 }
 
@@ -143,14 +136,7 @@ irqs_reset(vfu_ctx_t *vfu_ctx)
     irqs_disable(vfu_ctx, VFIO_PCI_ERR_IRQ_INDEX, 0, 0);
 
     for (i = 0; i < vfu_ctx->irqs->max_ivs; i++) {
-        if (efds[i] >= 0) {
-            if (close(efds[i]) == -1) {
-                vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m",
-                        efds[i]);
-            }
-
-            efds[i] = -1;
-        }
+        close_safely(&efds[i]);
     }
 }
 
@@ -257,12 +243,7 @@ irqs_set_data_eventfd(vfu_ctx_t *vfu_ctx, struct vfio_irq_set *irq_set,
     for (i = irq_set->start, j = 0; i < (irq_set->start + irq_set->count);
          i++, j++) {
         efd = irqs_get_efd(vfu_ctx, irq_set->index, i);
-        if (*efd >= 0) {
-            if (close(*efd) == -1) {
-                vfu_log(vfu_ctx, LOG_DEBUG, "failed to close IRQ fd %d: %m", *efd);
-            }
-            *efd = -1;
-        }
+        close_safely(efd);
         assert(data[j] >= 0);
         /*
          * We've already checked in handle_device_set_irqs that

diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
@@ -755,12 +755,9 @@ handle_dma_map(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
                                     dma_map->size, fd, dma_map->offset,
                                     prot);
     if (ret < 0) {
-        ret = errno;
         vfu_log(vfu_ctx, LOG_ERR, "failed to add DMA region %s: %m", rstr);
-        if (fd != -1) {
-            close(fd);
-        }
-        return ERROR_INT(ret);
+        close_safely(&fd);
+        return -1;
     }
 
     if (vfu_ctx->dma_register != NULL) {
@@ -1096,14 +1093,12 @@ free_msg(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     free(msg->in.iov.iov_base);
 
     for (i = 0; i < msg->in.nr_fds; i++) {
-        if (msg->in.fds[i] != -1) {
-            if (msg->processed_cmd) {
-                vfu_log(vfu_ctx, LOG_DEBUG,
-                        "closing unexpected fd %d (index %zu) from cmd %u",
-                        msg->in.fds[i], i, msg->hdr.cmd);
-            }
-            close(msg->in.fds[i]);
+        if (msg->in.fds[i] != -1 && msg->processed_cmd) {
+            vfu_log(vfu_ctx, LOG_DEBUG,
+                    "closing unexpected fd %d (index %zu) from cmd %u",
+                    msg->in.fds[i], i, msg->hdr.cmd);
         }
+        close_safely(&msg->in.fds[i]);
     }
 
     free(msg->in.fds);
@@ -1276,11 +1271,9 @@ get_request_header(vfu_ctx_t *vfu_ctx, vfu_msg_t **msgp)
     *msgp = alloc_msg(&hdr, fds, nr_fds);
 
     if (*msgp == NULL) {
-        int saved_errno = errno;
         for (i = 0; i < nr_fds; i++) {
-            close(fds[i]);
+            close_safely(&fds[i]);
         }
-        errno = saved_errno;
         return -1;
     }
 

diff --git a/lib/tran.c b/lib/tran.c
@@ -252,9 +252,7 @@ recv_version(vfu_ctx_t *vfu_ctx, uint16_t *msg_idp,
         (void) vfu_ctx->tran->reply(vfu_ctx, &rmsg, ret);
 
         for (i = 0; i < msg.in.nr_fds; i++) {
-            if (msg.in.fds[i] != -1) {
-                close(msg.in.fds[i]);
-            }
+            close_safely(&msg.in.fds[i]);
         }
 
         free(msg.in.iov.iov_base);

diff --git a/lib/tran_sock.c b/lib/tran_sock.c
@@ -419,8 +419,8 @@ tran_sock_init(vfu_ctx_t *vfu_ctx)
 
 out:
     if (ret != 0) {
-        if (ts != NULL && ts->listen_fd != -1) {
-            close(ts->listen_fd);
+        if (ts != NULL) {
+            close_safely(&ts->listen_fd);
         }
         free(ts);
         return ERROR_INT(ret);
@@ -466,10 +466,8 @@ tran_sock_attach(vfu_ctx_t *vfu_ctx)
 
     ret = tran_negotiate(vfu_ctx);
     if (ret < 0) {
-        ret = errno;
-        close(ts->conn_fd);
-        ts->conn_fd = -1;
-        return ERROR_INT(ret);
+        close_safely(&ts->conn_fd);
+        return -1;
     }
 
     return 0;
@@ -636,10 +634,8 @@ tran_sock_detach(vfu_ctx_t *vfu_ctx)
 
     ts = vfu_ctx->tran_data;
 
-    if (ts != NULL && ts->conn_fd != -1) {
-        // FIXME: handle EINTR
-        (void) close(ts->conn_fd);
-        ts->conn_fd = -1;
+    if (ts != NULL) {
+        close_safely(&ts->conn_fd);
     }
 }
 
@@ -654,11 +650,7 @@ tran_sock_fini(vfu_ctx_t *vfu_ctx)
 
     if (ts != NULL) {
         (void) unlink(vfu_ctx->uuid);
-        if (ts->listen_fd != -1) {
-            // FIXME: handle EINTR
-            (void) close(ts->listen_fd);
-            ts->listen_fd = -1;
-        }
+        close_safely(&ts->listen_fd);
     }
 
     free(vfu_ctx->tran_data);