From 02eae9f3ba65ab28838f39a4b0d7fdf57fbe83a5 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Tue, 15 Jul 2025 11:07:38 +0100
Subject: [PATCH] Fix Coverity warnings

Two problems were found:-
1) A useless test in scp_list.c - testing an unsigned int was >= 0.
2) Flow control issues in scp.c:scp_get_connect_session_response() meant
   that file descriptors could be leaked. A helper function has been
   used to simplify the code.
---
 libipm/scp.c      | 117 +++++++++++++++++++++++++++++-----------------
 sesman/scp_list.c |   3 +-
 sesman/scp_list.h |   3 +-
 3 files changed, 78 insertions(+), 45 deletions(-)

diff --git a/libipm/scp.c b/libipm/scp.c
index 298d526c..6e48dbd6 100644
--- a/libipm/scp.c
+++ b/libipm/scp.c
@@ -607,6 +607,58 @@ scp_send_connect_session_response(struct trans *trans,
     return rv;
 }
 
+/*****************************************************************************/
+/**
+ * Helper function to get the file descriptors for a connect
+ *
+ * @param trans SCP trans
+ * @param[out] display_fd Display server file descriptor
+ * @param[out] chan_fd Chansrv file descriptor
+ * @return != 0 for error
+ *
+ * This wrapper is nneded as libipm doesn't currently guarantee to
+ * handle received file descriptors well if an error is encountered
+ * mid-message.
+ *
+ * If an error is returned, some file descriptors may be valid.
+ */
+static int
+get_connect_session_response_fds(struct trans *trans,
+                                 int *display_fd,
+                                 int *chan_fd)
+{
+    int rv;
+    int fd_present;
+
+    // Read the display server file descriptor and guard
+    if ((rv = libipm_msg_in_parse(trans, "b", &fd_present)) != 0)
+    {
+        return rv;
+    }
+    if (fd_present)
+    {
+        if ((rv = libipm_msg_in_parse(trans, "h", display_fd)) != 0)
+        {
+            return rv;
+        }
+    }
+
+    // Read the chansrv file descriptor and guard
+    if ((rv = libipm_msg_in_parse(trans, "b", &fd_present)) != 0)
+    {
+        return rv;
+    }
+    if (fd_present)
+    {
+        if ((rv = libipm_msg_in_parse(trans, "h", chan_fd)) != 0)
+        {
+            return rv;
+        }
+    }
+
+    return 0;
+}
+
 /*****************************************************************************/
 
 int
@@ -615,59 +667,40 @@ scp_get_connect_session_response(struct trans *trans,
                                  int *display_fd,
                                  int *chan_fd)
 {
-    int fd_present;
-    int got_display_fd = 0;
-    int got_chan_fd = 0;
-
+    int rv;
     /* Intermediate values */
     int32_t i_status;
-    int rv = libipm_msg_in_parse( trans, "i", &i_status);
 
-    // Read the X11 file descriptor
-    if (rv == 0 && libipm_msg_in_parse(trans, "b", &fd_present) == 0)
+    /* Set the returned FDs to nonsensical values to stop valid
+     * FDs getting clobbered */
+    *display_fd = -1;
+    *chan_fd = -1;
+
+    if ((rv = libipm_msg_in_parse( trans, "i", &i_status)) == 0)
     {
-        if (!fd_present)
+        // Us a helper function to get the file descriptors as this
+        // makes flow control easier.
+        rv = get_connect_session_response_fds(trans, display_fd, chan_fd);
+        if (rv == 0)
         {
-            // Caller didn't send display_fd
-            *display_fd = -1;
+            *status = (enum scp_sconnect_status)i_status;
         }
         else
         {
-            got_display_fd = (libipm_msg_in_parse(trans, "h", display_fd) == 0);
+            // Close any fds we did receive to stop leaks
+            if (*display_fd >= 0)
+            {
+                g_file_close(*display_fd);
+                *display_fd = -1;
+            }
+            if (*chan_fd >= 0)
+            {
+                g_file_close(*chan_fd);
+                *chan_fd = -1;
+            }
         }
     }
 
-    // Read the chansrv file descriptor
-    if (rv == 0 && libipm_msg_in_parse(trans, "b", &fd_present) == 0)
-    {
-        if (!fd_present)
-        {
-            // Caller didn't send chan_fd
-            *chan_fd = -1;
-        }
-        else
-        {
-            got_chan_fd = (libipm_msg_in_parse(trans, "h", chan_fd) == 0);
-        }
-    }
-
-    // If we've failed, close any file descriptors we've parsed so far
-    if (rv != 0)
-    {
-        if (got_display_fd)
-        {
-            g_file_close(*display_fd);
-        }
-        if (got_chan_fd)
-        {
-            g_file_close(*chan_fd);
-        }
-    }
-    else
-    {
-        *status = (enum scp_sconnect_status)i_status;
-    }
-
     return rv;
 }
 
diff --git a/sesman/scp_list.c b/sesman/scp_list.c
index 3e96bdd2..ab278d1b 100644
--- a/sesman/scp_list.c
+++ b/sesman/scp_list.c
@@ -273,8 +273,7 @@ scp_list_get_create_session_displays(struct set_int *alloc_displays)
         sli = (struct scp_list_item *)list_get_item(g_scp_list, i);
 
         if (SCP_LIST_ITEM_IN_USE(sli) &&
-                sli->create_session_in_progress &&
-                sli->session_display >= 0)
+                sli->create_session_in_progress)
         {
             set_int_add(alloc_displays, sli->session_display);
         }
diff --git a/sesman/scp_list.h b/sesman/scp_list.h
index 0752d329..92230a16 100644
--- a/sesman/scp_list.h
+++ b/sesman/scp_list.h
@@ -86,7 +86,8 @@ struct scp_list_item
     char start_ip_addr[MAX_PEER_ADDRSTRLEN];
     int is_admin;
     int create_session_in_progress; ///< Already handling a create_session
-    unsigned int session_display;  ///< Display allocated for create_session
+    /// Display allocated for session. This is always valid (>= 0)
+    unsigned int session_display;
 };