From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Check return value of pclose() correctly
Date: Tue, 8 Nov 2022 14:14:14 +0100
Message-ID: <d599afd7-71df-604c-7307-cd8febadef48@enterprisedb.com> (raw)
In-Reply-To: <1735b462-b178-f2aa-5e73-182634a04a7b@enterprisedb.com>
References: <8cd9fb02-bc26-65f1-a809-b1cb360eef73@enterprisedb.com>
<Y2CwITuzlNpF/dWj@paquier.xyz>
<1735b462-b178-f2aa-5e73-182634a04a7b@enterprisedb.com>
On 01.11.22 21:30, Peter Eisentraut wrote:
>>> 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.
>>
>> Hmm. I would leave these out, I think. setQFout() relies on the
>> result of openQueryOutputFile(). And this could make commands like
>> \watch less reliable.
>
> I don't quite understand what you are saying here. My point is that,
> for example, setQFout() thinks it's important to check the result of
> popen() and write an error message, but it doesn't check the result of
> pclose() at all. I don't think that makes sense in practice.
I have looked this over again. In these cases, if the piped-to command
is faulty, you get a "broken pipe" error anyway, so the effect of not
checking the pclose() result is negligible. So I have removed the
"FIXME" markers without further action.
There is also the question whether we want to check the exit status of a
user-supplied command, such as in pgbench's \setshell. I have dialed
back my patch there, since I don't know what the current practice in
pgbench scripts is. If the command fails badly, pgbench will probably
complain anyway about invalid output.
More important is that something like pg_upgrade does check the exit
status when it calls pg_controldata etc. That's what this patch
accomplishes.
Attachments:v2-0001-Check-return-value-of-pclose-correctly.patch (text/plain, 6.9K) download
From 86b2b61d30f848b84c69437c7106dea8fdf3738d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 8 Nov 2022 14:03:12 +0100
Subject: [PATCH v2] 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.
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 | 6 +++++-
8 files changed, 50 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.cindex 86fbc7fa019f..fbf45f05aa0f 100644--- a/src/backend/commands/collationcmds.c+++ b/src/backend/commands/collationcmds.c@@ -639,6 +639,7 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
int naliases,
maxaliases,
i;
+ int pclose_rc;
/* expansible array of aliases */
maxaliases = 100;
@@ -745,7 +746,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.cindex 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.cindex 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.cindex 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.cindex 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.cindex b208d74767f1..36905a896817 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.cindex 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/common/wait_error.c b/src/common/wait_error.cindex 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
base-commit: bd95816f74ad4cad3d2a3c160be426358d6cea51
--
2.38.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, michael@paquier.xyz
Subject: Re: Check return value of pclose() correctly
In-Reply-To: <d599afd7-71df-604c-7307-cd8febadef48@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox