Skip to content

Commit 4e81886

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 3dfba9f commit 4e81886

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
@@ -195,10 +195,10 @@ stop_postmaster_atexit(void)
195195
bool
196196
start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
197197
{
198+
char cmd[MAXPGPATH * 4 + 1000];
198199
PGconn *conn;
199200
bool pg_ctl_return = false;
200-
PQExpBufferData cmd;
201-
PQExpBufferData opts;
201+
char socket_string[MAXPGPATH + 200];
202202

203203
static bool exit_hook_registered = false;
204204

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

211-
initPQExpBuffer(&cmd);
211+
socket_string[0] = '\0';
212212

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

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

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

290253
/*
291254
* Don't throw an error right away, let connecting throw the error because
@@ -297,7 +260,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
297260
SERVER_START_LOG_FILE) != 0) ?
298261
SERVER_LOG_FILE : NULL,
299262
report_and_exit_on_error, false,
300-
"%s", cmd.data);
263+
"%s", cmd);
301264

302265
/* Did it fail and we are just testing if the server could be started? */
303266
if (!pg_ctl_return && !report_and_exit_on_error)
@@ -335,14 +298,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
335298
if (cluster == &old_cluster)
336299
pg_fatal("could not connect to source postmaster started with the command:\n"
337300
"%s\n",
338-
cmd.data);
301+
cmd);
339302
else
340303
pg_fatal("could not connect to target postmaster started with the command:\n"
341304
"%s\n",
342-
cmd.data);
305+
cmd);
343306
}
344307
PQfinish(conn);
345-
termPQExpBuffer(&cmd);
346308

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

0 commit comments

Comments
 (0)