[Peter Eisentraut <peter.eisentraut@enterprisedb.com>]

I noticed that some (not all) 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().  Here is a patch 
with fixes.

(A failure to run the command issued by popen() is usually reported via 
the pclose() status, so while you can often get away with not checking 
fclose() or close(), checking pclose() is more often useful.)

There are some places where the return value is apparently intentionally 
ignored, such as in error recovery paths, or psql ignoring a failure to 
launch the pager.  (The intention can usually be inferred by the kind of 
error checking attached to the corresponding popen() call.)  But there 
are a few places in psql that I'm suspicious about that I have marked, 
but need to think about further.

Attachments:

v1-0001-Check-return-value-of-pclose-correctly.patch (text/plain, 7.8K) download
From b7a75cb25939b4930ddd4f7a99119144cccd4c98 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 31 Oct 2022 09:04:45 +0100
Subject: [PATCH v1] 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.
---
 src/backend/commands/collationcmds.c | 11 ++++++++++-
 src/bin/pg_ctl/pg_ctl.c              |  3 +--
 src/bin/pg_upgrade/controldata.c     | 12 +++++++++---
 src/bin/pg_upgrade/exec.c            |  6 +++++-
 src/bin/pg_upgrade/option.c          |  6 +++++-
 src/bin/pgbench/pgbench.c            |  6 ++++--
 src/bin/psql/command.c               | 18 ++++++++++++++----
 src/bin/psql/common.c                |  3 +++
 src/common/wait_error.c              |  6 +++++-
 9 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index fcfc02d2aede..3b30801f9344 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -605,6 +605,7 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
 		int			naliases,
 					maxaliases,
 					i;
+		int			pclose_rc;
 
 		/* expansible array of aliases */
 		maxaliases = 100;
@@ -711,7 +712,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 a509fc9109e8..ceab603c47d9 100644
--- 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 018cd310f7c8..73bfd14397cf 100644
--- 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 60f4b5443e68..23fe50e33d9c 100644
--- 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 3f719f1762c5..f441668c612a 100644
--- 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 b208d74767f1..2d5dba890839 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -2927,6 +2927,7 @@ runShellCommand(Variables *variables, char *variable, char **argv, int argc)
 	int			i,
 				len = 0;
 	FILE	   *fp;
+	int			rc;
 	char		res[64];
 	char	   *endptr;
 	int			retval;
@@ -3000,9 +3001,10 @@ runShellCommand(Variables *variables, char *variable, char **argv, int argc)
 		(void) pclose(fp);
 		return false;
 	}
-	if (pclose(fp) < 0)
+	if ((rc = pclose(fp)) != 0)
 	{
-		pg_log_error("%s: could not close shell command", argv[0]);
+		pg_log_error("%s: could not run shell command: %s", argv[0],
+					 wait_result_to_str(rc));
 		return false;
 	}
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index ab613dd49e0a..6958562e3340 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2694,14 +2694,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/bin/psql/common.c b/src/bin/psql/common.c
index 864f195992f5..f46524b4cfd9 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -103,6 +103,7 @@ setQFout(const char *fname)
 	if (pset.queryFout && pset.queryFout != stdout && pset.queryFout != stderr)
 	{
 		if (pset.queryFoutPipe)
+			// FIXME?
 			pclose(pset.queryFout);
 		else
 			fclose(pset.queryFout);
@@ -1635,6 +1636,7 @@ ExecQueryAndProcessResults(const char *query,
 	{
 		if (gfile_is_pipe)
 		{
+			// FIXME?
 			pclose(gfile_fout);
 			restore_sigpipe_trap();
 		}
@@ -1851,6 +1853,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
 		/* close \g argument file/pipe */
 		if (is_pipe)
 		{
+			// FIXME?
 			pclose(fout);
 			restore_sigpipe_trap();
 		}
diff --git a/src/common/wait_error.c b/src/common/wait_error.c
index a776f2901e8e..8397e238b2b4 100644
--- a/src/common/wait_error.c
+++ b/src/common/wait_error.c
@@ -33,7 +33,11 @@ wait_result_to_str(int exitstatus)
 {
 	char		str[512];
 
-	if (WIFEXITED(exitstatus))
+	if (exitstatus == -1)
+	{
+		snprintf(str, sizeof(str), "%m");
+	}
+	else if (WIFEXITED(exitstatus))
 	{
 		/*
 		 * Give more specific error message for some common exit codes that
-- 
2.37.3