From 00f51830dc8fea742b7bbecbf57f8fd3c92921a5 Mon Sep 17 00:00:00 2001
From: Xi Luo <xi.luo@intel.com>
Date: Wed, 25 Oct 2023 09:58:07 -0700
Subject: [PATCH] util: fix defects when gathering coordinates

This commit addresses the following issues:
Resource leaks in pmix_init()
Error handling issues in parse_coord_file()
Insecure data handling in parse_coord_file()
---
 src/util/mpir_pmi.c    | 24 ++++++++++++++----------
 src/util/mpir_pmix.inc | 15 +++++++--------
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/src/util/mpir_pmi.c b/src/util/mpir_pmi.c
index 497be1b762d..0c6e2fcb43e 100644
--- a/src/util/mpir_pmi.c
+++ b/src/util/mpir_pmi.c
@@ -855,7 +855,7 @@ static void free_pmi_keyvals(INFO_TYPE ** kv, int size, int *counts)
 static int parse_coord_file(const char *filename)
 {
     int mpi_errno = MPI_SUCCESS;
-    int i, j, rank;
+    int i, j, rank, coords_dims;
     FILE *coords_file;
     int fields_scanned;
 
@@ -864,31 +864,36 @@ static int parse_coord_file(const char *filename)
                          "**filenoexist", "**filenoexist %s", filename);
 
     /* Skip the first line */
-    fields_scanned = fscanf(coords_file, "%*[^\n]\n");
-    MPIR_Process.coords_dims = 0;
+    int fields_scanned = fscanf(coords_file, "%*[^\n]\n");
+    MPIR_ERR_CHKANDSTMT2(0 != fields_scanned, mpi_errno, MPI_ERR_FILE, goto fn_fail_read,
+                         "**read_file", "**read_file %s %s", filename, strerror(errno));
+    coords_dims = 0;
     fields_scanned = fscanf(coords_file, "%d:", &rank);
     MPIR_ERR_CHKANDSTMT2(1 != fields_scanned, mpi_errno, MPI_ERR_FILE, goto fn_fail_read,
                          "**read_file", "**read_file %s %s", filename, strerror(errno));
     while (!feof(coords_file)) {
         int temp = 0;
         if (fscanf(coords_file, "%d", &temp) == 1)
-            ++MPIR_Process.coords_dims;
+            ++coords_dims;
         else
             break;
         if (fgetc(coords_file) == '\n')
             break;
     }
 
-    MPIR_Assert(MPIR_Process.coords_dims == 3);
+    MPIR_Assert(coords_dims == 3);
+    MPIR_Process.coords_dims = coords_dims;
     rewind(coords_file);
     /* Skip the first line */
     fields_scanned = fscanf(coords_file, "%*[^\n]\n");
+    MPIR_ERR_CHKANDSTMT2(0 != fields_scanned, mpi_errno, MPI_ERR_FILE, goto fn_fail_read,
+                         "**read_file", "**read_file %s %s", filename, strerror(errno));
 
     if (MPIR_Process.coords == NULL) {
         MPIR_Process.coords =
-            MPL_malloc(MPIR_Process.coords_dims * sizeof(int) * MPIR_Process.size, MPL_MEM_COLL);
+            MPL_malloc(coords_dims * sizeof(int) * MPIR_Process.size, MPL_MEM_COLL);
     }
-    memset(MPIR_Process.coords, -1, MPIR_Process.coords_dims * sizeof(int) * MPIR_Process.size);
+    memset(MPIR_Process.coords, -1, coords_dims * sizeof(int) * MPIR_Process.size);
 
     for (i = 0; i < MPIR_Process.size; ++i) {
         fields_scanned = fscanf(coords_file, "%d:", &rank);
@@ -900,12 +905,11 @@ static int parse_coord_file(const char *filename)
                         rank, MPIR_Process.size);
             continue;
         }
-        for (j = 0; j < MPIR_Process.coords_dims; ++j) {
+        for (j = 0; j < coords_dims; ++j) {
             /* MPIR_Process.coords stores the coords in this order: port number, switch_id, group_id */
             fields_scanned =
                 fscanf(coords_file, "%d",
-                       &MPIR_Process.coords[rank * MPIR_Process.coords_dims +
-                                            MPIR_Process.coords_dims - 1 - j]);
+                       &MPIR_Process.coords[rank * coords_dims + coords_dims - 1 - j]);
             MPIR_ERR_CHKANDSTMT2(1 != fields_scanned, mpi_errno, MPI_ERR_FILE, goto fn_fail_read,
                                  "**read_file", "**read_file %s %s", filename, strerror(errno));
         }
diff --git a/src/util/mpir_pmix.inc b/src/util/mpir_pmix.inc
index 089fded51b3..6b1e356ab6c 100644
--- a/src/util/mpir_pmix.inc
+++ b/src/util/mpir_pmix.inc
@@ -98,18 +98,16 @@ static int pmix_init(int *has_parent, int *rank, int *size, int *appnum)
     MPIR_Assert(pvalue->data.uint32 <= INT_MAX);        /* overflow check */
     *appnum = (int) pvalue->data.uint32;
     PMIX_VALUE_RELEASE(pvalue);
-    pmix_value_t value;
-    pmix_value_t *val = &value;
 
     for (int i = 0; i < *size; i++) {
         pmix_proc.rank = i;
-        pmi_errno = PMIx_Get(&pmix_proc, PMIX_FABRIC_COORDINATES, NULL, 0, &val);
+        pmi_errno = PMIx_Get(&pmix_proc, PMIX_FABRIC_COORDINATES, NULL, 0, &pvalue);
         if (pmi_errno != PMIX_SUCCESS) {
             MPIR_Process.coords = NULL;
             break;
         }
-        MPIR_Assert(val->data.coord->dims <= INT_MAX);
-        MPIR_Process.coords_dims = (int) val->data.coord->dims;
+        MPIR_Assert(pvalue->data.coord->dims <= INT_MAX);
+        MPIR_Process.coords_dims = (int) pvalue->data.coord->dims;
         MPIR_Assert(MPIR_Process.coords_dims == 3);
 
         if (i == 0) {
@@ -118,13 +116,14 @@ static int pmix_init(int *has_parent, int *rank, int *size, int *appnum)
             MPIR_ERR_CHKANDJUMP(!MPIR_Process.coords, mpi_errno, MPI_ERR_OTHER, "**nomem");
         }
 
-        if (PMIX_COORD == val->type) {
+        if (PMIX_COORD == pvalue->type) {
             for (int j = 0; j < MPIR_Process.coords_dims; j++) {
-                /* MPIR_Process.coords stores the coords in this order: port number, switch_id, group_id */
+                /* MPIR_Process.coords is in this order: port number, switch_id, group_id */
                 MPIR_Process.coords[i * MPIR_Process.coords_dims + j] =
-                    val->data.coord->coord[MPIR_Process.coords_dims - 1 - j];
+                    pvalue->data.coord->coord[MPIR_Process.coords_dims - 1 - j];
             }
         }
+        PMIX_VALUE_RELEASE(pvalue);
     }
 
   fn_exit: