public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniel Gustafsson <[email protected]>
To: Postgres hackers <[email protected]>
Subject: pipe_read_line for reading arbitrary strings
Date: Tue, 7 Mar 2023 23:05:12 +0100
Message-ID: <[email protected]> (raw)
When skimming through pg_rewind during a small review I noticed the use of
pipe_read_line for reading arbitrary data from a pipe, the mechanics of which
seemed odd.
Commit 5b2f4afffe6 refactored find_other_exec() and broke out pipe_read_line()
as a static convenience routine for reading a single line of output to catch a
version number. Many years later, commit a7e8ece41 exposed it externally in
order to read a GUC from postgresql.conf using "postgres -C ..". f06b1c598
also make use of it for reading a version string much like find_other_exec().
Funnily enough, while now used for arbitrary string reading the variable is
still "pgver".
Since the function requires passing a buffer/size, and at most size - 1 bytes
will be read via fgets(), there is a truncation risk when using this for
reading GUCs (like how pg_rewind does, though the risk there is slim to none).
If we are going to continue using this for reading $stuff from pipes, maybe we
should think about presenting a nicer API which removes that risk? Returning
an allocated buffer which contains all the output along the lines of the recent
pg_get_line work seems a lot nicer and safer IMO.
The attached POC diff replace fgets() with pg_get_line(), which may not be an
Ok way to cross the streams (it's clearly not a great fit), but as a POC it
provided a neater interface for reading one-off lines from a pipe IMO. Does
anyone else think this is worth fixing before too many callsites use it, or is
this another case of my fear of silent subtle truncation bugs? =)
--
Daniel Gustafsson
Attachments:
[application/octet-stream] pipe_read_line.diff (4.6K, ../[email protected]/2-pipe_read_line.diff)
download | inline diff:
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index f7f3b8227f..038543defb 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -1042,8 +1042,7 @@ static void
getRestoreCommand(const char *argv0)
{
int rc;
- char postgres_exec_path[MAXPGPATH],
- cmd_output[MAXPGPATH];
+ char postgres_exec_path[MAXPGPATH];
PQExpBuffer postgres_cmd;
if (!restore_wal)
@@ -1092,16 +1091,15 @@ getRestoreCommand(const char *argv0)
/* add -C switch, for restore_command */
appendPQExpBufferStr(postgres_cmd, " -C restore_command");
- if (!pipe_read_line(postgres_cmd->data, cmd_output, sizeof(cmd_output)))
+ restore_command = pipe_read_line(postgres_cmd->data);
+ if (restore_command == NULL)
exit(1);
- (void) pg_strip_crlf(cmd_output);
+ (void) pg_strip_crlf(restore_command);
- if (strcmp(cmd_output, "") == 0)
+ if (strcmp(restore_command, "") == 0)
pg_fatal("restore_command is not set in the target cluster");
- restore_command = pg_strdup(cmd_output);
-
pg_log_debug("using for rewind restore_command = \'%s\'",
restore_command);
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index 5b2edebe41..f56f22af4e 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -431,7 +431,7 @@ static void
check_exec(const char *dir, const char *program, bool check_version)
{
char path[MAXPGPATH];
- char line[MAXPGPATH];
+ char *line;
char cmd[MAXPGPATH];
char versionstr[128];
@@ -442,7 +442,7 @@ check_exec(const char *dir, const char *program, bool check_version)
snprintf(cmd, sizeof(cmd), "\"%s\" -V", path);
- if (!pipe_read_line(cmd, line, sizeof(line)))
+ if ((line = pipe_read_line(cmd)) == NULL)
pg_fatal("check for \"%s\" failed: cannot execute",
path);
@@ -456,4 +456,6 @@ check_exec(const char *dir, const char *program, bool check_version)
pg_fatal("check for \"%s\" failed: incorrect version: found \"%s\", expected \"%s\"",
path, line, versionstr);
}
+
+ pg_free(line);
}
diff --git a/src/common/exec.c b/src/common/exec.c
index 93b2faf2c4..99130fed11 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -33,6 +33,8 @@
#endif
#endif
+#include "common/string.h"
+
/* Inhibit mingw CRT's auto-globbing of command line arguments */
#if defined(WIN32) && !defined(_MSC_VER)
extern int _CRT_glob = 0; /* 0 turns off globbing; 1 turns it on */
@@ -352,7 +354,7 @@ find_other_exec(const char *argv0, const char *target,
const char *versionstr, char *retpath)
{
char cmd[MAXPGPATH];
- char line[MAXPGPATH];
+ char *line;
if (find_my_exec(argv0, retpath) < 0)
return -1;
@@ -370,46 +372,48 @@ find_other_exec(const char *argv0, const char *target,
snprintf(cmd, sizeof(cmd), "\"%s\" -V", retpath);
- if (!pipe_read_line(cmd, line, sizeof(line)))
+ if ((line = pipe_read_line(cmd)) == NULL)
return -1;
if (strcmp(line, versionstr) != 0)
+ {
+ pfree(line);
return -2;
+ }
+ pfree(line);
return 0;
}
/*
- * Execute a command in a pipe and read the first line from it.
+ * Execute a command in a pipe and read the first line from it. The returned
+ * string is allocated, the caller is responsible for freeing.
*/
char *
-pipe_read_line(char *cmd, char *line, int maxsize)
+pipe_read_line(char *cmd)
{
- FILE *pgver;
+ FILE *pipe_cmd;
+ char *line;
fflush(NULL);
errno = 0;
- if ((pgver = popen(cmd, "r")) == NULL)
+ if ((pipe_cmd = popen(cmd, "r")) == NULL)
{
perror("popen failure");
return NULL;
}
- errno = 0;
- if (fgets(line, maxsize, pgver) == NULL)
+ line = pg_get_line(pipe_cmd, NULL);
+
+ if (line == NULL)
{
- if (feof(pgver))
- fprintf(stderr, "no data was returned by command \"%s\"\n", cmd);
- else
- perror("fgets failure");
- pclose(pgver); /* no error checking */
- return NULL;
+ log_error(errcode_for_file_access(),
+ _("no data was returned by command \"%s\""), cmd);
}
- if (pclose_check(pgver))
- return NULL;
+ (void) pclose_check(pipe_cmd);
return line;
}
diff --git a/src/include/port.h b/src/include/port.h
index e66193bed9..6eac3757d3 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -137,7 +137,7 @@ extern int validate_exec(const char *path);
extern int find_my_exec(const char *argv0, char *retpath);
extern int find_other_exec(const char *argv0, const char *target,
const char *versionstr, char *retpath);
-extern char *pipe_read_line(char *cmd, char *line, int maxsize);
+extern char *pipe_read_line(char *cmd);
/* Doesn't belong here, but this is used with find_other_exec(), so... */
#define PG_BACKEND_VERSIONSTR "postgres (PostgreSQL) " PG_VERSION "\n"
view thread (2+ messages)
reply
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: [email protected]
Cc: [email protected], [email protected]
Subject: Re: pipe_read_line for reading arbitrary strings
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox