public inbox for [email protected]
help / color / mirror / Atom feedFrom: Daniel Gustafsson <[email protected]>
To: Postgres hackers <[email protected]>
Subject: Re: pipe_read_line for reading arbitrary strings
Date: Wed, 17 May 2023 13:49:54 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
> On 7 Mar 2023, at 23:05, Daniel Gustafsson <[email protected]> wrote:
>
> 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.
A rebase of this for the CFBot since I realized I had forgotten to add this to
the July CF.
--
Daniel Gustafsson
Attachments:
[application/octet-stream] v2-0001-Refactor-pipe_read_line-to-return-the-full-line.patch (6.1K, ../[email protected]/2-v2-0001-Refactor-pipe_read_line-to-return-the-full-line.patch)
download | inline diff:
From a66759e24b3cc597f964ec9f9189251dab5f3ca6 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email protected]>
Date: Wed, 17 May 2023 12:55:55 +0200
Subject: [PATCH v2] Refactor pipe_read_line to return the full line
Commit 5b2f4afffe6 refactored find_other_exec() and in the process broke
out pipe_read_line() into a static routine for reading a single line of
output, aimed at reading a version number. Commit a7e8ece41 later 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(). The internal variable remained "pgver" though.
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 in this case is
marginal).
To keep this as generic functionality for reading a line from a pipe, this
refactors pipe_read_line into returning an allocated buffer containing all
of the line to remove the risk of silent truncation.
Discussion: https://postgr.es/m/[email protected]
---
src/bin/pg_rewind/pg_rewind.c | 14 ++++++--------
src/bin/pg_upgrade/exec.c | 6 ++++--
src/common/exec.c | 36 +++++++++++++++++++----------------
src/include/port.h | 2 +-
4 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index f7f3b8227f..c9fbe740e7 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)))
- exit(1);
+ restore_command = pipe_read_line(postgres_cmd->data);
+ if (restore_command == NULL)
+ pg_fatal("unable to read restore_command from target cluster");
- (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 f209b934df..70c9397643 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -42,6 +42,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 */
@@ -328,7 +330,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;
@@ -346,46 +348,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 a88d403483..76ef003a69 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"
--
2.32.1 (Apple Git-133)
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