Refactor pipe_read_line to return the full line

Commit 5b2f4afffe6 refactored find_other_exec() and in the process
created pipe_read_line() into a static routine for reading a single
line of output, aimed at reading version numbers.  Commit a7e8ece41
later exposed it externally in order to read a postgresql.conf GUC
using "postgres -C ..".  Further, f06b1c598 also made use of it for
reading a version string much like find_other_exec().  The internal
variable remained "pgver", even when used for other purposes.

Since the function requires passing a buffer and its 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.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: John Naylor <johncnaylorls@gmail.com>
Discussion: https://postgr.es/m/DEDF73CE-D528-49A3-9089-B3592FD671A9@yesql.se

1 files changed, 25 insertions, 14 deletions

diff --git a/src/common/exec.c b/src/common/exec.c
index 8cfc721b12a..da929f15b95 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,55 @@ 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 palloc'd (malloc'd in frontend code), 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;
 	}
 
+	/* Make sure popen() didn't change errno */
 	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);
+		if (ferror(pipe_cmd))
+			log_error(errcode_for_file_access(),
+					  _("could not read from command \"%s\": %m"), 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\": %m"), cmd);
 	}
 
-	if (pclose_check(pgver))
-		return NULL;
+	(void) pclose_check(pipe_cmd);
 
 	return line;
 }