From 2fe3bdbd691a5d11626308e7d660440be6c210c8 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Tue, 15 Nov 2022 15:35:37 +0100 Subject: [PATCH] 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 Discussion: https://www.postgresql.org/message-id/flat/8cd9fb02-bc26-65f1-a809-b1cb360eef73@enterprisedb.com --- 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 | 2 +- src/bin/psql/command.c | 18 ++++++++++++++---- src/common/wait_error.c | 14 ++++++++++++-- 8 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c index 25efa6e0bf..e1429c7bf5 100644 --- 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 a509fc9109..ceab603c47 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 018cd310f7..73bfd14397 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 60f4b5443e..23fe50e33d 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 3f719f1762..f441668c61 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 b208d74767..36905a8968 100644 --- 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 3b06169ba0..7672ed9e9d 100644 --- 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 a776f2901e..411c8008ff 100644 --- 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