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

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 57a168fea2da0f3be2b4dd3741e30f5783f26c90..bde90bf60bb0bdb4ee16738fb81459c80b546fcf 100644 (file)
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -1055,8 +1055,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)
@@ -1105,16 +1104,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 fec8dc4c2f735c02f4401586fd04ea47483cf2d4..3552cf00afbf794fdb13d4c465b8de2361634c27 100644 (file)
--- 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 8cfc721b12a2e597535ba0918b151b9c935fc444..da929f15b959ddb0dd608c18004464eb816e8cbb 100644 (file)
--- 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;
 }

diff --git a/src/include/port.h b/src/include/port.h
index 2e3425da63843625318607ba1eabe06a6bef3fd7..263cf791a392259909bc45a9a5f8c44c643afd21 100644 (file)
--- 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"