pg_upgrade: Fix quoting of some arguments in pg_ctl command

michaelpq · michaelpq · commit 770eca3adfe4 · 2020-02-10T10:49:38.000+09:00
The previous coding forgot to apply shell quoting to the socket
directory and the data folder, leading to failures when running
pg_upgrade.  This refactors the code generating the pg_ctl command
starting clusters to use a more correct shell quoting.  Failures are
easier to trigger in 12 and newer versions by using a value of
--socketdir that includes quotes, but it is also possible to cause
failures with quotes included in the default socket directory used by
pg_upgrade or the data folders of the clusters involved in the
upgrade.

As 9.4 is going to be EOL'd with the next minor release, nobody is
likely going to upgrade to it now so this branch is not included in the
set of branches fixed.

Author: Michael Paquier
Reviewed-by: Álvaro Herrera, Noah Misch
Backpatch-through: 9.5
diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
@@ -196,10 +196,10 @@ stop_postmaster_atexit(void)
 bool
 start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 {
-	char		cmd[MAXPGPATH * 4 + 1000];
 	PGconn	   *conn;
 	bool		pg_ctl_return = false;
-	char		socket_string[MAXPGPATH + 200];
+	PQExpBufferData cmd;
+	PQExpBufferData opts;
 
 	static bool exit_hook_registered = false;
 
@@ -209,22 +209,28 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 		exit_hook_registered = true;
 	}
 
-	socket_string[0] = '\0';
+	initPQExpBuffer(&cmd);
 
-#ifdef HAVE_UNIX_SOCKETS
-	/* prevent TCP/IP connections, restrict socket access */
-	strcat(socket_string,
-		   " -c listen_addresses='' -c unix_socket_permissions=0700");
+	/* Path to pg_ctl */
+	appendPQExpBuffer(&cmd, "\"%s/pg_ctl\" -w ", cluster->bindir);
 
-	/* Have a sockdir?	Tell the postmaster. */
-	if (cluster->sockdir)
-		snprintf(socket_string + strlen(socket_string),
-				 sizeof(socket_string) - strlen(socket_string),
-				 " -c %s='%s'",
-				 (GET_MAJOR_VERSION(cluster->major_version) < 903) ?
-				 "unix_socket_directory" : "unix_socket_directories",
-				 cluster->sockdir);
-#endif
+	/* log file */
+	appendPQExpBufferStr(&cmd, "-l ");
+	appendShellString(&cmd, SERVER_LOG_FILE);
+	appendPQExpBufferChar(&cmd, ' ');
+
+	/* data folder */
+	appendPQExpBufferStr(&cmd, "-D ");
+	appendShellString(&cmd, cluster->pgconfig);
+	appendPQExpBufferChar(&cmd, ' ');
+
+	/*
+	 * Build set of options for the instance to start.  These are
+	 * handled with a separate string as they are one argument in
+	 * the command produced to which shell quoting needs to be applied.
+	 */
+	initPQExpBuffer(&opts);
+	appendPQExpBuffer(&opts, "-p %d ", cluster->port);
 
 	/*
 	 * Since PG 9.1, we have used -b to disable autovacuum.  For earlier
@@ -235,21 +241,52 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * is no need to set that.)  We assume all datfrozenxid and relfrozenxid
 	 * values are less than a gap of 2000000000 from the current xid counter,
 	 * so autovacuum will not touch them.
-	 *
+	 */
+	if (cluster->controldata.cat_ver >= BINARY_UPGRADE_SERVER_FLAG_CAT_VER)
+		appendPQExpBufferStr(&opts, "-b ");
+	else
+		appendPQExpBufferStr(&opts,
+							 "-c autovacuum=off "
+							 "-c autovacuum_freeze_max_age=2000000000 ");
+
+	/*
 	 * Turn off durability requirements to improve object creation speed, and
 	 * we only modify the new cluster, so only use it there.  If there is a
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 */
-	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
-			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
-			 (cluster->controldata.cat_ver >=
-			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-			 " -c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
-			 (cluster == &new_cluster) ?
-			 " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "",
-			 cluster->pgopts ? cluster->pgopts : "", socket_string);
+	if (cluster == &new_cluster)
+		appendPQExpBufferStr(&opts,
+							 "-c synchronous_commit=off "
+							 "-c fsync=off "
+							 "-c full_page_writes=off ");
+
+	if (cluster->pgopts)
+		appendPQExpBufferStr(&opts, cluster->pgopts);
+
+#ifdef HAVE_UNIX_SOCKETS
+	appendPQExpBuffer(&opts,
+		"-c listen_addresses='' -c unix_socket_permissions=0700 ");
+
+	/* Have a sockdir?	Tell the postmaster. */
+	if (cluster->sockdir)
+	{
+		appendPQExpBuffer(&opts,
+				" -c %s=",
+				(GET_MAJOR_VERSION(cluster->major_version) < 903) ?
+				"unix_socket_directory" : "unix_socket_directories");
+		appendPQExpBufferStr(&opts, cluster->sockdir);
+		appendPQExpBufferChar(&opts, ' ');
+	}
+#endif
+
+	/* Apply shell quoting to the option string */
+	appendPQExpBufferStr(&cmd, "-o ");
+	appendShellString(&cmd, opts.data);
+	termPQExpBuffer(&opts);
+
+	/* Start mode for pg_ctl */
+	appendPQExpBufferStr(&cmd, " start");
 
 	/*
 	 * Don't throw an error right away, let connecting throw the error because
@@ -261,7 +298,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 									  SERVER_START_LOG_FILE) != 0) ?
 							  SERVER_LOG_FILE : NULL,
 							  report_and_exit_on_error, false,
-							  "%s", cmd);
+							  "%s", cmd.data);
 
 	/* Did it fail and we are just testing if the server could be started? */
 	if (!pg_ctl_return && !report_and_exit_on_error)
@@ -299,13 +336,14 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 		if (cluster == &old_cluster)
 			pg_fatal("could not connect to source postmaster started with the command:\n"
 					 "%s\n",
-					 cmd);
+					 cmd.data);
 		else
 			pg_fatal("could not connect to target postmaster started with the command:\n"
 					 "%s\n",
-					 cmd);
+					 cmd.data);
 	}
 	PQfinish(conn);
+	termPQExpBuffer(&cmd);
 
 	/*
 	 * If pg_ctl failed, and the connection didn't fail, and
