Skip to content

Commit b3f5cce

Browse files
tglsfdctglsfdc
committed
Make pg_createsubscriber more wary about quoting connection parameters.
The original coding here could fail with database names, user names, etc that contain spaces or other special characters. As partial test coverage, extend the 040_pg_createsubscriber.pl test script so that it uses a generated database name containing funny characters. Hayato Kuroda (some mods by me), per complaint from Noah Misch Discussion: https://postgr.es/m/20240623062157.97.nmisch@google.com
1 parent db0c96c commit b3f5cce

File tree

2 files changed

+96
-68
lines changed

2 files changed

+96
-68
lines changed

‎src/bin/pg_basebackup/pg_createsubscriber.c‎

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "common/restricted_token.h"
2727
#include "fe_utils/recovery_gen.h"
2828
#include "fe_utils/simple_list.h"
29+
#include "fe_utils/string_utils.h"
2930
#include "getopt_long.h"
3031

3132
#define DEFAULT_SUB_PORT "50432"
@@ -236,15 +237,28 @@ usage(void)
236237
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
237238
}
238239

240+
/*
241+
* Subroutine to append "keyword=value" to a connection string,
242+
* with proper quoting of the value. (We assume keywords don't need that.)
243+
*/
244+
static void
245+
appendConnStrItem(PQExpBuffer buf, const char *keyword, const char *val)
246+
{
247+
if (buf->len > 0)
248+
appendPQExpBufferChar(buf, ' ');
249+
appendPQExpBufferStr(buf, keyword);
250+
appendPQExpBufferChar(buf, '=');
251+
appendConnStrVal(buf, val);
252+
}
253+
239254
/*
240255
* Validate a connection string. Returns a base connection string that is a
241256
* connection string without a database name.
242257
*
243258
* Since we might process multiple databases, each database name will be
244259
* appended to this base connection string to provide a final connection
245260
* string. If the second argument (dbname) is not null, returns dbname if the
246-
* provided connection string contains it. If option --database is not
247-
* provided, uses dbname as the only database to setup the logical replica.
261+
* provided connection string contains it.
248262
*
249263
* It is the caller's responsibility to free the returned connection string and
250264
* dbname.
@@ -257,7 +271,6 @@ get_base_conninfo(const char *conninfo, char **dbname)
257271
PQconninfoOption *conn_opt;
258272
char *errmsg = NULL;
259273
char *ret;
260-
int i;
261274

262275
conn_opts = PQconninfoParse(conninfo, &errmsg);
263276
if (conn_opts == NULL)
@@ -268,22 +281,17 @@ get_base_conninfo(const char *conninfo, char **dbname)
268281
}
269282

270283
buf = createPQExpBuffer();
271-
i = 0;
272284
for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
273285
{
274-
if (strcmp(conn_opt->keyword, "dbname") == 0 && conn_opt->val != NULL)
275-
{
276-
if (dbname)
277-
*dbname = pg_strdup(conn_opt->val);
278-
continue;
279-
}
280-
281286
if (conn_opt->val != NULL && conn_opt->val[0] != '\0')
282287
{
283-
if (i > 0)
284-
appendPQExpBufferChar(buf, ' ');
285-
appendPQExpBuffer(buf, "%s=%s", conn_opt->keyword, conn_opt->val);
286-
i++;
288+
if (strcmp(conn_opt->keyword, "dbname") == 0)
289+
{
290+
if (dbname)
291+
*dbname = pg_strdup(conn_opt->val);
292+
continue;
293+
}
294+
appendConnStrItem(buf, conn_opt->keyword, conn_opt->val);
287295
}
288296
}
289297

@@ -305,13 +313,13 @@ get_sub_conninfo(const struct CreateSubscriberOptions *opt)
305313
PQExpBuffer buf = createPQExpBuffer();
306314
char *ret;
307315

308-
appendPQExpBuffer(buf, "port=%s", opt->sub_port);
316+
appendConnStrItem(buf, "port", opt->sub_port);
309317
#if !defined(WIN32)
310-
appendPQExpBuffer(buf, " host=%s", opt->socket_dir);
318+
appendConnStrItem(buf, "host", opt->socket_dir);
311319
#endif
312320
if (opt->sub_username != NULL)
313-
appendPQExpBuffer(buf, " user=%s", opt->sub_username);
314-
appendPQExpBuffer(buf, " fallback_application_name=%s", progname);
321+
appendConnStrItem(buf, "user", opt->sub_username);
322+
appendConnStrItem(buf, "fallback_application_name", progname);
315323

316324
ret = pg_strdup(buf->data);
317325

@@ -402,7 +410,7 @@ concat_conninfo_dbname(const char *conninfo, const char *dbname)
402410
Assert(conninfo != NULL);
403411

404412
appendPQExpBufferStr(buf, conninfo);
405-
appendPQExpBuffer(buf, " dbname=%s", dbname);
413+
appendConnStrItem(buf, "dbname", dbname);
406414

407415
ret = pg_strdup(buf->data);
408416
destroyPQExpBuffer(buf);
@@ -1312,8 +1320,9 @@ start_standby_server(const struct CreateSubscriberOptions *opt, bool restricted_
13121320
PQExpBuffer pg_ctl_cmd = createPQExpBuffer();
13131321
int rc;
13141322

1315-
appendPQExpBuffer(pg_ctl_cmd, "\"%s\" start -D \"%s\" -s -o \"-c sync_replication_slots=off\"",
1316-
pg_ctl_path, subscriber_dir);
1323+
appendPQExpBuffer(pg_ctl_cmd, "\"%s\" start -D ", pg_ctl_path);
1324+
appendShellString(pg_ctl_cmd, subscriber_dir);
1325+
appendPQExpBuffer(pg_ctl_cmd, " -s -o \"-c sync_replication_slots=off\"");
13171326
if (restricted_access)
13181327
{
13191328
appendPQExpBuffer(pg_ctl_cmd, " -o \"-p %s\"", opt->sub_port);

‎src/bin/pg_basebackup/t/040_pg_createsubscriber.pl‎

Lines changed: 65 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,27 @@
1515

1616
my $datadir = PostgreSQL::Test::Utils::tempdir;
1717

18+
# Generate a database with a name made of a range of ASCII characters.
19+
# Extracted from 002_pg_upgrade.pl.
20+
sub generate_db
21+
{
22+
my ($node, $prefix, $from_char, $to_char, $suffix) = @_;
23+
24+
my $dbname = $prefix;
25+
for my $i ($from_char .. $to_char)
26+
{
27+
next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
28+
$dbname = $dbname . sprintf('%c', $i);
29+
}
30+
31+
$dbname .= $suffix;
32+
$node->command_ok(
33+
[ 'createdb', $dbname ],
34+
"created database with ASCII characters from $from_char to $to_char");
35+
36+
return $dbname;
37+
}
38+
1839
#
1940
# Test mandatory options
2041
command_fails(['pg_createsubscriber'],
@@ -104,16 +125,14 @@
104125
# - create test tables
105126
# - insert a row
106127
# - create a physical replication slot
107-
$node_p->safe_psql(
108-
'postgres', q(
109-
CREATE DATABASE pg1;
110-
CREATE DATABASE pg2;
111-
));
112-
$node_p->safe_psql('pg1', 'CREATE TABLE tbl1 (a text)');
113-
$node_p->safe_psql('pg1', "INSERT INTO tbl1 VALUES('first row')");
114-
$node_p->safe_psql('pg2', 'CREATE TABLE tbl2 (a text)');
128+
my $db1 = generate_db($node_p, 'regression\\"\\', 1, 45, '\\\\"\\\\\\');
129+
my $db2 = generate_db($node_p, 'regression', 46, 90, '');
130+
131+
$node_p->safe_psql($db1, 'CREATE TABLE tbl1 (a text)');
132+
$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES('first row')");
133+
$node_p->safe_psql($db2, 'CREATE TABLE tbl2 (a text)');
115134
my $slotname = 'physical_slot';
116-
$node_p->safe_psql('pg2',
135+
$node_p->safe_psql($db2,
117136
"SELECT pg_create_physical_replication_slot('$slotname')");
118137

119138
# Set up node S as standby linking to node P
@@ -143,11 +162,11 @@
143162
'pg_createsubscriber', '--verbose',
144163
'--dry-run', '--pgdata',
145164
$node_t->data_dir, '--publisher-server',
146-
$node_p->connstr('pg1'), '--socket-directory',
165+
$node_p->connstr($db1), '--socket-directory',
147166
$node_t->host, '--subscriber-port',
148167
$node_t->port, '--database',
149-
'pg1', '--database',
150-
'pg2'
168+
$db1, '--database',
169+
$db2
151170
],
152171
'target server is not in recovery');
153172

@@ -157,11 +176,11 @@
157176
'pg_createsubscriber', '--verbose',
158177
'--dry-run', '--pgdata',
159178
$node_s->data_dir, '--publisher-server',
160-
$node_p->connstr('pg1'), '--socket-directory',
179+
$node_p->connstr($db1), '--socket-directory',
161180
$node_s->host, '--subscriber-port',
162181
$node_s->port, '--database',
163-
'pg1', '--database',
164-
'pg2'
182+
$db1, '--database',
183+
$db2
165184
],
166185
'standby is up and running');
167186

@@ -170,11 +189,11 @@
170189
[
171190
'pg_createsubscriber', '--verbose',
172191
'--pgdata', $node_f->data_dir,
173-
'--publisher-server', $node_p->connstr('pg1'),
192+
'--publisher-server', $node_p->connstr($db1),
174193
'--socket-directory', $node_f->host,
175194
'--subscriber-port', $node_f->port,
176-
'--database', 'pg1',
177-
'--database', 'pg2'
195+
'--database', $db1,
196+
'--database', $db2
178197
],
179198
'subscriber data directory is not a copy of the source database cluster');
180199

@@ -191,16 +210,16 @@
191210
'pg_createsubscriber', '--verbose',
192211
'--dry-run', '--pgdata',
193212
$node_c->data_dir, '--publisher-server',
194-
$node_s->connstr('pg1'), '--socket-directory',
213+
$node_s->connstr($db1), '--socket-directory',
195214
$node_c->host, '--subscriber-port',
196215
$node_c->port, '--database',
197-
'pg1', '--database',
198-
'pg2'
216+
$db1, '--database',
217+
$db2
199218
],
200219
'primary server is in recovery');
201220

202221
# Insert another row on node P and wait node S to catch up
203-
$node_p->safe_psql('pg1', "INSERT INTO tbl1 VALUES('second row')");
222+
$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES('second row')");
204223
$node_p->wait_for_replay_catchup($node_s);
205224

206225
# Check some unmet conditions on node P
@@ -218,11 +237,11 @@
218237
'pg_createsubscriber', '--verbose',
219238
'--dry-run', '--pgdata',
220239
$node_s->data_dir, '--publisher-server',
221-
$node_p->connstr('pg1'), '--socket-directory',
240+
$node_p->connstr($db1), '--socket-directory',
222241
$node_s->host, '--subscriber-port',
223242
$node_s->port, '--database',
224-
'pg1', '--database',
225-
'pg2'
243+
$db1, '--database',
244+
$db2
226245
],
227246
'primary contains unmet conditions on node P');
228247
# Restore default settings here but only apply it after testing standby. Some
@@ -247,11 +266,11 @@
247266
'pg_createsubscriber', '--verbose',
248267
'--dry-run', '--pgdata',
249268
$node_s->data_dir, '--publisher-server',
250-
$node_p->connstr('pg1'), '--socket-directory',
269+
$node_p->connstr($db1), '--socket-directory',
251270
$node_s->host, '--subscriber-port',
252271
$node_s->port, '--database',
253-
'pg1', '--database',
254-
'pg2'
272+
$db1, '--database',
273+
$db2
255274
],
256275
'standby contains unmet conditions on node S');
257276
$node_s->append_conf(
@@ -265,7 +284,7 @@
265284

266285
# Create failover slot to test its removal
267286
my $fslotname = 'failover_slot';
268-
$node_p->safe_psql('pg1',
287+
$node_p->safe_psql($db1,
269288
"SELECT pg_create_logical_replication_slot('$fslotname', 'pgoutput', false, false, true)");
270289
$node_s->start;
271290
$node_s->safe_psql('postgres', "SELECT pg_sync_replication_slots()");
@@ -280,15 +299,15 @@
280299
'--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
281300
'--dry-run', '--pgdata',
282301
$node_s->data_dir, '--publisher-server',
283-
$node_p->connstr('pg1'), '--socket-directory',
302+
$node_p->connstr($db1), '--socket-directory',
284303
$node_s->host, '--subscriber-port',
285304
$node_s->port, '--publication',
286305
'pub1', '--publication',
287306
'pub2', '--subscription',
288307
'sub1', '--subscription',
289308
'sub2', '--database',
290-
'pg1', '--database',
291-
'pg2'
309+
$db1, '--database',
310+
$db2
292311
],
293312
'run pg_createsubscriber --dry-run on node S');
294313

@@ -304,7 +323,7 @@
304323
'pg_createsubscriber', '--verbose',
305324
'--dry-run', '--pgdata',
306325
$node_s->data_dir, '--publisher-server',
307-
$node_p->connstr('pg1'), '--socket-directory',
326+
$node_p->connstr($db1), '--socket-directory',
308327
$node_s->host, '--subscriber-port',
309328
$node_s->port, '--replication-slot',
310329
'replslot1'
@@ -318,29 +337,29 @@
318337
'--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
319338
'--verbose', '--pgdata',
320339
$node_s->data_dir, '--publisher-server',
321-
$node_p->connstr('pg1'), '--socket-directory',
340+
$node_p->connstr($db1), '--socket-directory',
322341
$node_s->host, '--subscriber-port',
323342
$node_s->port, '--publication',
324343
'pub1', '--publication',
325344
'Pub2', '--replication-slot',
326345
'replslot1', '--replication-slot',
327346
'replslot2', '--database',
328-
'pg1', '--database',
329-
'pg2'
347+
$db1, '--database',
348+
$db2
330349
],
331350
'run pg_createsubscriber on node S');
332351

333352
# Confirm the physical replication slot has been removed
334-
$result = $node_p->safe_psql('pg1',
353+
$result = $node_p->safe_psql($db1,
335354
"SELECT count(*) FROM pg_replication_slots WHERE slot_name = '$slotname'"
336355
);
337356
is($result, qq(0),
338357
'the physical replication slot used as primary_slot_name has been removed'
339358
);
340359

341360
# Insert rows on P
342-
$node_p->safe_psql('pg1', "INSERT INTO tbl1 VALUES('third row')");
343-
$node_p->safe_psql('pg2', "INSERT INTO tbl2 VALUES('row 1')");
361+
$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES('third row')");
362+
$node_p->safe_psql($db2, "INSERT INTO tbl2 VALUES('row 1')");
344363

345364
# Start subscriber
346365
$node_s->start;
@@ -357,20 +376,20 @@
357376
$node_s->wait_for_subscription_sync($node_p, $subnames[1]);
358377

359378
# Confirm the failover slot has been removed
360-
$result = $node_s->safe_psql('pg1',
379+
$result = $node_s->safe_psql($db1,
361380
"SELECT count(*) FROM pg_replication_slots WHERE slot_name = '$fslotname'");
362381
is($result, qq(0), 'failover slot was removed');
363382

364-
# Check result on database pg1
365-
$result = $node_s->safe_psql('pg1', 'SELECT * FROM tbl1');
383+
# Check result on database $db1
384+
$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1');
366385
is( $result, qq(first row
367386
second row
368387
third row),
369-
'logical replication works on database pg1');
388+
"logical replication works on database $db1");
370389

371-
# Check result on database pg2
372-
$result = $node_s->safe_psql('pg2', 'SELECT * FROM tbl2');
373-
is($result, qq(row 1), 'logical replication works on database pg2');
390+
# Check result on database $db2
391+
$result = $node_s->safe_psql($db2, 'SELECT * FROM tbl2');
392+
is($result, qq(row 1), "logical replication works on database $db2");
374393

375394
# Different system identifier?
376395
my $sysid_p = $node_p->safe_psql('postgres',

0 commit comments

Comments
 (0)