From: Daniel Gustafsson <daniel@yesql.se>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_703AD2F4-305F-4686-9A97-3749774A8DB1"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.2\))
Subject: pipe_read_line for reading arbitrary strings
Message-Id: <DEDF73CE-D528-49A3-9089-B3592FD671A9@yesql.se>
Date: Tue, 7 Mar 2023 23:05:12 +0100
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Archived-At: <https://www.postgresql.org/message-id/DEDF73CE-D528-49A3-9089-B3592FD671A9%40yesql.se>
Precedence: bulk


--Apple-Mail=_703AD2F4-305F-4686-9A97-3749774A8DB1
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

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?  =3D)

--
Daniel Gustafsson


--Apple-Mail=_703AD2F4-305F-4686-9A97-3749774A8DB1
Content-Disposition: attachment;
	filename=pipe_read_line.diff
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="pipe_read_line.diff"
Content-Transfer-Encoding: 7bit

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"

--Apple-Mail=_703AD2F4-305F-4686-9A97-3749774A8DB1--