Check return value of pclose() correctly

Some callers didn't check the return value of pclose() or
ClosePipeStream() correctly.  Either they didn't check it at all or
they treated it like the return of fclose().

The correct way is to first check whether the return value is -1, and
then report errno, and then check the return value like a result from
system(), for which we already have wait_result_to_str() to make it
simpler.  To make this more compact, expand wait_result_to_str() to
also handle -1 explicitly.

Reviewed-by: Ankit Kumar Pandey <itsankitkp@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/8cd9fb02-bc26-65f1-a809-b1cb360eef73@enterprisedb.com

diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index 25efa6e0bf0c51890e8c75b280d4c5e9951ec1ff..e1429c7bf5fea3d24f578c6f714bec43de09eb23 100644 (file)
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -640,6 +640,7 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
        int         naliases,
                    maxaliases,
                    i;
+       int         pclose_rc;
 
        /* expansible array of aliases */
        maxaliases = 100;
@@ -746,7 +747,15 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
            }
        }
 
-       ClosePipeStream(locale_a_handle);
+       pclose_rc = ClosePipeStream(locale_a_handle);
+       if (pclose_rc != 0)
+       {
+           ereport(ERROR,
+                   (errcode_for_file_access(),
+                    errmsg("could not execute command \"%s\": %s",
+                           "locale -a",
+                           wait_result_to_str(pclose_rc))));
+       }
 
        /*
         * Before processing the aliases, sort them by locale name.  The point

diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index a509fc9109e84db2fe37f4eef8f225adc9497046..ceab603c47d93e11bba571f33fdb44d69a310398 100644 (file)
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2154,12 +2154,11 @@ adjust_data_dir(void)
    fflush(NULL);
 
    fd = popen(cmd, "r");
-   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL)
+   if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL || pclose(fd) != 0)
    {
        write_stderr(_("%s: could not determine the data directory using command \"%s\"\n"), progname, cmd);
        exit(1);
    }
-   pclose(fd);
    free(my_exec_path);
 
    /* strip trailing newline and carriage return */

diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index 018cd310f7c8b3d380c2389d98c86701058b8c33..73bfd14397cfde250953ed05db96dad9b9535b3a 100644 (file)
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -75,7 +75,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
    uint32      logid = 0;
    uint32      segno = 0;
    char       *resetwal_bin;
-
+   int         rc;
 
    /*
     * Because we test the pg_resetwal output as strings, it has to be in
@@ -170,7 +170,10 @@ get_control_data(ClusterInfo *cluster, bool live_check)
            }
        }
 
-       pclose(output);
+       rc = pclose(output);
+       if (rc != 0)
+           pg_fatal("could not get control data using %s: %s",
+                    cmd, wait_result_to_str(rc));
 
        if (!got_cluster_state)
        {
@@ -500,7 +503,10 @@ get_control_data(ClusterInfo *cluster, bool live_check)
        }
    }
 
-   pclose(output);
+   rc = pclose(output);
+   if (rc != 0)
+       pg_fatal("could not get control data using %s: %s",
+                cmd, wait_result_to_str(rc));
 
    /*
     * Restore environment variables.  Note all but LANG and LC_MESSAGES were

diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index 60f4b5443e68dd2930d425c74522c4a43934fefb..23fe50e33d9c906f2e7c971c7887415e8661d18e 100644 (file)
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -35,6 +35,7 @@ get_bin_version(ClusterInfo *cluster)
    char        cmd[MAXPGPATH],
                cmd_output[MAX_STRING];
    FILE       *output;
+   int         rc;
    int         v1 = 0,
                v2 = 0;
 
@@ -46,7 +47,10 @@ get_bin_version(ClusterInfo *cluster)
        pg_fatal("could not get pg_ctl version data using %s: %s",
                 cmd, strerror(errno));
 
-   pclose(output);
+   rc = pclose(output);
+   if (rc != 0)
+       pg_fatal("could not get pg_ctl version data using %s: %s",
+                cmd, wait_result_to_str(rc));
 
    if (sscanf(cmd_output, "%*s %*s %d.%d", &v1, &v2) < 1)
        pg_fatal("could not get pg_ctl version output from %s", cmd);

diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 3f719f1762c591aefdea5800af51df05f8ddcbef..f441668c612afc120e2e068820275cf05e92859c 100644 (file)
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -384,6 +384,7 @@ adjust_data_dir(ClusterInfo *cluster)
                cmd_output[MAX_STRING];
    FILE       *fp,
               *output;
+   int         rc;
 
    /* Initially assume config dir and data dir are the same */
    cluster->pgconfig = pg_strdup(cluster->pgdata);
@@ -423,7 +424,10 @@ adjust_data_dir(ClusterInfo *cluster)
        pg_fatal("could not get data directory using %s: %s",
                 cmd, strerror(errno));
 
-   pclose(output);
+   rc = pclose(output);
+   if (rc != 0)
+       pg_fatal("could not get control data directory using %s: %s",
+                cmd, wait_result_to_str(rc));
 
    /* strip trailing newline and carriage return */
    (void) pg_strip_crlf(cmd_output);

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b208d74767f197b603aef1e1370496d550c0b330..36905a896817858db15a954057a2e21b5a64e42a 100644 (file)
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3002,7 +3002,7 @@ runShellCommand(Variables *variables, char *variable, char **argv, int argc)
    }
    if (pclose(fp) < 0)
    {
-       pg_log_error("%s: could not close shell command", argv[0]);
+       pg_log_error("%s: could not run shell command: %m", argv[0]);
        return false;
    }
 

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 3b06169ba0dcab5fb032a998759877eeb925e3a1..7672ed9e9d567c55c4d9e2c309ad1a3e0372d33a 100644 (file)
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2731,14 +2731,24 @@ exec_command_write(PsqlScanState scan_state, bool active_branch,
                fprintf(fd, "%s\n", previous_buf->data);
 
            if (is_pipe)
+           {
                result = pclose(fd);
+
+               if (result != 0)
+               {
+                   pg_log_error("%s: %s", fname, wait_result_to_str(result));
+                   status = PSQL_CMD_ERROR;
+               }
+           }
            else
+           {
                result = fclose(fd);
 
-           if (result == EOF)
-           {
-               pg_log_error("%s: %m", fname);
-               status = PSQL_CMD_ERROR;
+               if (result == EOF)
+               {
+                   pg_log_error("%s: %m", fname);
+                   status = PSQL_CMD_ERROR;
+               }
            }
        }
 

diff --git a/src/common/wait_error.c b/src/common/wait_error.c
index a776f2901e8e1cb6b8fa1bd3003f4de80add3129..411c8008ff0cec6b40b9932a95cbd3d330b55e3c 100644 (file)
--- a/src/common/wait_error.c
+++ b/src/common/wait_error.c
@@ -26,14 +26,24 @@
 /*
  * Return a human-readable string explaining the reason a child process
  * terminated. The argument is a return code returned by wait(2) or
- * waitpid(2). The result is a translated, palloc'd or malloc'd string.
+ * waitpid(2), which also applies to pclose(3) and system(3). The result is a
+ * translated, palloc'd or malloc'd string.
  */
 char *
 wait_result_to_str(int exitstatus)
 {
    char        str[512];
 
-   if (WIFEXITED(exitstatus))
+   /*
+    * To simplify using this after pclose() and system(), handle status -1
+    * first.  In that case, there is no wait result but some error indicated
+    * by errno.
+    */
+   if (exitstatus == -1)
+   {
+       snprintf(str, sizeof(str), "%m");
+   }
+   else if (WIFEXITED(exitstatus))
    {
        /*
         * Give more specific error message for some common exit codes that