Skip to content

Commit 198f0ba

Browse files
michaelpqmichaelpq
committed
Revert "pg_upgrade: Fix quoting of some arguments in pg_ctl command"
This reverts commit d1c0b61. The patch has some downsides that require more attention, as discussed with Noah Misch. Backpatch-through: 9.5
1 parent a6a8616 commit 198f0ba

File tree

1 file changed

+29
-67
lines changed

1 file changed

+29
-67
lines changed

‎src/bin/pg_upgrade/server.c

Lines changed: 29 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -196,10 +196,10 @@ stop_postmaster_atexit(void)
196196
bool
197197
start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
198198
{
199+
char cmd[MAXPGPATH * 4 + 1000];
199200
PGconn *conn;
200201
bool pg_ctl_return = false;
201-
PQExpBufferData cmd;
202-
PQExpBufferData opts;
202+
char socket_string[MAXPGPATH + 200];
203203

204204
static bool exit_hook_registered = false;
205205

@@ -209,28 +209,22 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
209209
exit_hook_registered = true;
210210
}
211211

212-
initPQExpBuffer(&cmd);
212+
socket_string[0] = '\0';
213213

214-
/* Path to pg_ctl */
215-
appendPQExpBuffer(&cmd, "\"%s/pg_ctl\" -w ", cluster->bindir);
216-
217-
/* log file */
218-
appendPQExpBufferStr(&cmd, "-l ");
219-
appendShellString(&cmd, SERVER_LOG_FILE);
220-
appendPQExpBufferChar(&cmd, ' ');
221-
222-
/* data folder */
223-
appendPQExpBufferStr(&cmd, "-D ");
224-
appendShellString(&cmd, cluster->pgconfig);
225-
appendPQExpBufferChar(&cmd, ' ');
214+
#ifdef HAVE_UNIX_SOCKETS
215+
/* prevent TCP/IP connections, restrict socket access */
216+
strcat(socket_string,
217+
" -c listen_addresses='' -c unix_socket_permissions=0700");
226218

227-
/*
228-
* Build set of options for the instance to start. These are
229-
* handled with a separate string as they are one argument in
230-
* the command produced to which shell quoting needs to be applied.
231-
*/
232-
initPQExpBuffer(&opts);
233-
appendPQExpBuffer(&opts, "-p %d ", cluster->port);
219+
/* Have a sockdir? Tell the postmaster. */
220+
if (cluster->sockdir)
221+
snprintf(socket_string + strlen(socket_string),
222+
sizeof(socket_string) - strlen(socket_string),
223+
" -c %s='%s'",
224+
(GET_MAJOR_VERSION(cluster->major_version) < 903) ?
225+
"unix_socket_directory" : "unix_socket_directories",
226+
cluster->sockdir);
227+
#endif
234228

235229
/*
236230
* Since PG 9.1, we have used -b to disable autovacuum. For earlier
@@ -241,52 +235,21 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
241235
* is no need to set that.) We assume all datfrozenxid and relfrozenxid
242236
* values are less than a gap of 2000000000 from the current xid counter,
243237
* so autovacuum will not touch them.
244-
*/
245-
if (cluster->controldata.cat_ver >= BINARY_UPGRADE_SERVER_FLAG_CAT_VER)
246-
appendPQExpBufferStr(&opts, "-b ");
247-
else
248-
appendPQExpBufferStr(&opts,
249-
"-c autovacuum=off "
250-
"-c autovacuum_freeze_max_age=2000000000 ");
251-
252-
/*
238+
*
253239
* Turn off durability requirements to improve object creation speed, and
254240
* we only modify the new cluster, so only use it there. If there is a
255241
* crash, the new cluster has to be recreated anyway. fsync=off is a big
256242
* win on ext4.
257243
*/
258-
if (cluster == &new_cluster)
259-
appendPQExpBufferStr(&opts,
260-
"-c synchronous_commit=off "
261-
"-c fsync=off "
262-
"-c full_page_writes=off ");
263-
264-
if (cluster->pgopts)
265-
appendPQExpBufferStr(&opts, cluster->pgopts);
266-
267-
#ifdef HAVE_UNIX_SOCKETS
268-
appendPQExpBuffer(&opts,
269-
"-c listen_addresses='' -c unix_socket_permissions=0700 ");
270-
271-
/* Have a sockdir? Tell the postmaster. */
272-
if (cluster->sockdir)
273-
{
274-
appendPQExpBuffer(&opts,
275-
" -c %s=",
276-
(GET_MAJOR_VERSION(cluster->major_version) < 903) ?
277-
"unix_socket_directory" : "unix_socket_directories");
278-
appendPQExpBufferStr(&opts, cluster->sockdir);
279-
appendPQExpBufferChar(&opts, ' ');
280-
}
281-
#endif
282-
283-
/* Apply shell quoting to the option string */
284-
appendPQExpBufferStr(&cmd, "-o ");
285-
appendShellString(&cmd, opts.data);
286-
termPQExpBuffer(&opts);
287-
288-
/* Start mode for pg_ctl */
289-
appendPQExpBufferStr(&cmd, " start");
244+
snprintf(cmd, sizeof(cmd),
245+
"\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
246+
cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
247+
(cluster->controldata.cat_ver >=
248+
BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
249+
" -c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
250+
(cluster == &new_cluster) ?
251+
" -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "",
252+
cluster->pgopts ? cluster->pgopts : "", socket_string);
290253

291254
/*
292255
* Don't throw an error right away, let connecting throw the error because
@@ -298,7 +261,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
298261
SERVER_START_LOG_FILE) != 0) ?
299262
SERVER_LOG_FILE : NULL,
300263
report_and_exit_on_error, false,
301-
"%s", cmd.data);
264+
"%s", cmd);
302265

303266
/* Did it fail and we are just testing if the server could be started? */
304267
if (!pg_ctl_return && !report_and_exit_on_error)
@@ -336,14 +299,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
336299
if (cluster == &old_cluster)
337300
pg_fatal("could not connect to source postmaster started with the command:\n"
338301
"%s\n",
339-
cmd.data);
302+
cmd);
340303
else
341304
pg_fatal("could not connect to target postmaster started with the command:\n"
342305
"%s\n",
343-
cmd.data);
306+
cmd);
344307
}
345308
PQfinish(conn);
346-
termPQExpBuffer(&cmd);
347309

348310
/*
349311
* If pg_ctl failed, and the connection didn't fail, and

0 commit comments

Comments
 (0)