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

Author: petere

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

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 */

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

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);

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);

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;
 	}
 

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;
+				}
 			}
 		}
 

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