On 2026-02-03 Tu 2:14 PM, Mahendra Singh Thalor wrote:
Thanks Álvaro, Tom and Nathan for the review.
On Mon, 2 Feb 2026 at 16:46, Álvaro Herrera <[email protected]> wrote:
+void
+reject_newline_in_name(const char *objname, const char *objtype)
+{
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(objname, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("\"%s\" name \"%s\" contains a newline or
carriage return character",objtype, objname));
+}
I think this error message doesn't work very well. Having the
"database" word be an untranslatable piece of the message makes no sense
to me. I would rather have the ereport() in each place where this is
needed so that we can have a complete phrase to translate, and avoid
this wrinkle. Alternatively you could pass the error message from the
caller (to abstract away the strpbrk() call) but I'm not sure that's
really all that useful.
Fixed.
BTW, looking at generate_db in src/bin/pg_upgrade/t/002_pg_upgrade.pl I
wonder why don't we reject BEL here also. (Or actually, maybe not, but
I think commit 322becb6085c was wrong to lose the part of the comment
that explained the reason.)
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -241,6 +241,18 @@ $node->command_fails(
],
'fails for invalid locale provider');
+$node->command_fails_like(
+ [ 'createdb', "invalid \n dbname" ],
+ qr(contains a newline or carriage return character),
+ 'fails if database name containing newline character in name'
+);
+
+$node->command_fails_like(
+ [ 'createdb', "invalid \r dbname" ],
+ qr(contains a newline or carriage return character),,
+ 'fails if database name containing carriage return character in name'
+);
Note there are two commas the the qr() line in the second stanza. Seems
to be innocuous, because the test log shows
Fixed.
# Running: createdb invalid
dbname
[11:57:00.942](0.012s) ok 34 - fails if database name containing newline
character in name: exit code not 0
[11:57:00.942](0.000s) ok 35 - fails if database name containing newline
character in name: matches
# Running: createdb invalid ^M dbname
[11:57:00.953](0.011s) ok 36 - fails if database name containing carriage
return character in name: exit code not 0
[11:57:00.954](0.000s) ok 37 - fails if database name containing carriage
return character in name: matches
but it'd look nicer without those commas. Also, the "fails if ...
containing" test names sound ungrammatical to me.
Fixed.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Always assume the user will do much worse than the stupidest thing
you can imagine." (Julien PUYDT)
Based on suggestions, I removed the new added function and changed the
error message and added a check for tablespace also.
Here, I am attaching updated patches.
OK, I have squashed these two together and tweaked them a bit. I propose
to commit this patch fairly soon.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 33311760df7..740c526e92d 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -743,6 +743,12 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
createdb_failure_params fparms;
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(dbname, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name \"%s\" contains a newline or carriage return character", dbname)));
+
/* Extract options from the statement node tree */
foreach(option, stmt->options)
{
@@ -1911,6 +1917,12 @@ RenameDatabase(const char *oldname, const char *newname)
int npreparedxacts;
ObjectAddress address;
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(newname, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("database name \"%s\" contains a newline or carriage return character", newname)));
+
/*
* Look up the target database's OID, and get exclusive lock on it. We
* need this for the same reasons as DROP DATABASE.
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 3511a4ec0fd..ed2a93a09db 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -242,6 +242,12 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
(errcode(ERRCODE_INVALID_NAME),
errmsg("tablespace location cannot contain single quotes")));
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(stmt->tablespacename, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("tablespace name \"%s\" contains a newline or carriage return character", stmt->tablespacename)));
+
in_place = allow_in_place_tablespaces && strlen(location) == 0;
/*
@@ -971,6 +977,12 @@ RenameTableSpace(const char *oldname, const char *newname)
errmsg("unacceptable tablespace name \"%s\"", newname),
errdetail("The prefix \"pg_\" is reserved for system tablespaces.")));
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(newname, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("tablespace name \"%s\" contains a newline or carriage return character", newname)));
+
/*
* If built with appropriate switch, whine when regression-testing
* conventions for tablespace names are violated.
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 8fb9ea25db0..be11c49f919 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -171,6 +171,12 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
DefElem *dbypassRLS = NULL;
GrantRoleOptions popt;
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(stmt->role, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name \"%s\" contains a newline or carriage return character", stmt->role)));
+
/* The defaults can vary depending on the original statement type */
switch (stmt->stmt_type)
{
@@ -1348,6 +1354,12 @@ RenameRole(const char *oldname, const char *newname)
ObjectAddress address;
Form_pg_authid authform;
+ /* Report error if name has \n or \r character. */
+ if (strpbrk(newname, "\n\r"))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("role name \"%s\" contains a newline or carriage return character", newname)));
+
rel = table_open(AuthIdRelationId, RowExclusiveLock);
dsc = RelationGetDescr(rel);
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index a8dcc2b5c75..bc7a082f57a 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2048,13 +2048,8 @@ my %tests = (
},
},
- 'newline of role or table name in comment' => {
- create_sql => qq{CREATE ROLE regress_newline;
- ALTER ROLE regress_newline SET enable_seqscan = off;
- ALTER ROLE regress_newline
- RENAME TO "regress_newline\nattack";
-
- -- meet getPartitioningInfo() "unsafe" condition
+ 'newline of table name in comment' => {
+ create_sql => qq{-- meet getPartitioningInfo() "unsafe" condition
CREATE TYPE pp_colors AS
ENUM ('green', 'blue', 'black');
CREATE TABLE pp_enumpart (a pp_colors)
diff --git a/src/bin/pg_dump/t/003_pg_dump_with_server.pl b/src/bin/pg_dump/t/003_pg_dump_with_server.pl
index 70f830e10dc..349add67d50 100644
--- a/src/bin/pg_dump/t/003_pg_dump_with_server.pl
+++ b/src/bin/pg_dump/t/003_pg_dump_with_server.pl
@@ -16,22 +16,6 @@ my $port = $node->port;
$node->init;
$node->start;
-#########################################
-# pg_dumpall: newline in database name
-
-$node->safe_psql('postgres', qq{CREATE DATABASE "regress_\nattack"});
-
-my (@cmd, $stdout, $stderr);
-@cmd = ("pg_dumpall", '--port' => $port, '--exclude-database=postgres');
-print("# Running: " . join(" ", @cmd) . "\n");
-my $result = IPC::Run::run \@cmd, '>' => \$stdout, '2>' => \$stderr;
-ok(!$result, "newline in dbname: exit code not 0");
-like(
- $stderr,
- qr/shell command argument contains a newline/,
- "newline in dbname: stderr matches");
-unlike($stdout, qr/^attack/m, "newline in dbname: no comment escape");
-
#########################################
# Verify that dumping foreign data includes only foreign tables of
# matching servers
diff --git a/src/bin/pg_dump/t/010_dump_connstr.pl b/src/bin/pg_dump/t/010_dump_connstr.pl
old mode 100644
new mode 100755
index dc7a33658db..bf2c3b6d00b
--- a/src/bin/pg_dump/t/010_dump_connstr.pl
+++ b/src/bin/pg_dump/t/010_dump_connstr.pl
@@ -153,20 +153,6 @@ $node->command_ok(
],
'pg_dumpall --dbname accepts connection string');
-$node->run_log(
- [ 'createdb', '--username' => $src_bootstrap_super, "foo\n\rbar" ]);
-
-# not sufficient to use --roles-only here
-$node->command_fails(
- [
- 'pg_dumpall', '--no-sync',
- '--username' => $src_bootstrap_super,
- '--file' => $discard,
- ],
- 'pg_dumpall with \n\r in database name');
-$node->run_log(
- [ 'dropdb', '--username' => $src_bootstrap_super, "foo\n\rbar" ]);
-
# make a table, so the parallel worker has something to dump
$node->safe_psql(
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 5c73773bf0e..c0e7ec35168 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -34,6 +34,7 @@ static void check_new_cluster_replication_slots(void);
static void check_new_cluster_subscription_configuration(void);
static void check_old_cluster_for_valid_slots(void);
static void check_old_cluster_subscription_state(void);
+static void check_old_cluster_global_names(ClusterInfo *cluster);
/*
* DataTypesUsageChecks - definitions of data type checks for the old cluster
@@ -600,6 +601,14 @@ check_and_dump_old_cluster(void)
*/
check_for_connection_status(&old_cluster);
+ /*
+ * Validate database, user, role and tablespace names from the old cluster.
+ * No need to check in 19 or newer as newline and carriage return are
+ * not allowed at the creation time of the object.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) < 1900)
+ check_old_cluster_global_names(&old_cluster);
+
/*
* Extract a list of databases, tables, and logical replication slots from
* the old cluster.
@@ -2500,3 +2509,73 @@ check_old_cluster_subscription_state(void)
else
check_ok();
}
+
+/*
+ * check_old_cluster_global_names()
+ *
+ * Raise an error if any database, role, or tablespace name contains a newline
+ * or carriage return character. Such names are not allowed in v19 and later.
+ */
+static void
+check_old_cluster_global_names(ClusterInfo *cluster)
+{
+ int i;
+ PGconn *conn_template1;
+ PGresult *res;
+ int ntups;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+ int count = 0;
+
+ prep_status("Checking names of databases, roles and tablespaces");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "db_role_tablespace_invalid_names.txt");
+
+ conn_template1 = connectToServer(cluster, "template1");
+
+ /*
+ * Get database, user/role and tablespacenames from cluster. Can't use
+ * pg_authid because only superusers can view it.
+ */
+ res = executeQueryOrDie(conn_template1,
+ "SELECT datname AS objname, 'database' AS objtype "
+ "FROM pg_catalog.pg_database UNION ALL "
+ "SELECT rolname AS objname, 'role' AS objtype "
+ "FROM pg_catalog.pg_roles UNION ALL "
+ "SELECT spcname AS objname, 'tablespace' AS objtype "
+ "FROM pg_catalog.pg_tablespace ORDER BY 2 ");
+
+ ntups = PQntuples(res);
+ for (i = 0; i < ntups; i++)
+ {
+ char *objname = PQgetvalue(res, i, 0);
+ char *objtype = PQgetvalue(res, i, 1);
+
+ /* If name has \n or \r, then report it. */
+ if (strpbrk(objname, "\n\r"))
+ {
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %m", output_path);
+
+ fprintf(script, "%d : %s name = \"%s\"\n", ++count, objtype, objname);
+ }
+ }
+
+ PQclear(res);
+ PQfinish(conn_template1);
+
+ if (script)
+ {
+ fclose(script);
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("All the database, role and tablespace names should have only valid characters. A newline or \n"
+ "carriage return character is not allowed in these object names. To fix this, please \n"
+ "rename these names with valid names. \n"
+ "To see all %d invalid object names, refer db_role_tablespace_invalid_names.txt file. \n"
+ " %s", count, output_path);
+ }
+ else
+ check_ok();
+}
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
old mode 100644
new mode 100755
index 83b0077383a..a0995868363
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -241,6 +241,18 @@ $node->command_fails(
],
'fails for invalid locale provider');
+$node->command_fails_like(
+ [ 'createdb', "invalid \n dbname" ],
+ qr(contains a newline or carriage return character),
+ 'fails if database name contains a newline character in name'
+);
+
+$node->command_fails_like(
+ [ 'createdb', "invalid \r dbname" ],
+ qr(contains a newline or carriage return character),
+ 'fails if database name contains a carriage return character in name'
+);
+
# Check use of templates with shared dependencies copied from the template.
my ($ret, $stdout, $stderr) = $node->psql(
'foobar2',
diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c
index 89ce396ed4f..38fffbd036b 100644
--- a/src/fe_utils/string_utils.c
+++ b/src/fe_utils/string_utils.c
@@ -568,12 +568,6 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
* Append the given string to the shell command being built in the buffer,
* with shell-style quoting as needed to create exactly one argument.
*
- * Forbid LF or CR characters, which have scant practical use beyond designing
- * security breaches. The Windows command shell is unusable as a conduit for
- * arguments containing LF or CR characters. A future major release should
- * reject those characters in CREATE ROLE and CREATE DATABASE, because use
- * there eventually leads to errors here.
- *
* appendShellString() simply prints an error and dies if LF or CR appears.
* appendShellStringNoError() omits those characters from the result, and
* returns false if there were any.
diff --git a/src/test/modules/unsafe_tests/expected/alter_system_table.out b/src/test/modules/unsafe_tests/expected/alter_system_table.out
index b73b9442b8d..9ea6061f4ae 100644
--- a/src/test/modules/unsafe_tests/expected/alter_system_table.out
+++ b/src/test/modules/unsafe_tests/expected/alter_system_table.out
@@ -64,6 +64,11 @@ ERROR: permission denied: "pg_description" is a system catalog
CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
ERROR: unacceptable tablespace name "pg_foo"
DETAIL: The prefix "pg_" is reserved for system tablespaces.
+-- contains \n\r tablespace name
+CREATE TABLESPACE "invalid
+name" LOCATION '/no/such/location';
+ERROR: tablespace name "invalid
+name" contains a newline or carriage return character
-- triggers
CREATE FUNCTION tf1() RETURNS trigger
LANGUAGE plpgsql
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 61396b2a805..9dfb8475e16 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -102,6 +102,10 @@ DETAIL: Role names starting with "pg_" are reserved.
CREATE ROLE "pg_abcdef"; -- error
ERROR: role name "pg_abcdef" is reserved
DETAIL: Role names starting with "pg_" are reserved.
+CREATE ROLE "invalid
+rolename"; -- error
+ERROR: role name "invalid
+rolename" contains a newline or carriage return character
CREATE ROLE regress_testrol0 SUPERUSER LOGIN;
CREATE ROLE regress_testrolx SUPERUSER LOGIN;
CREATE ROLE regress_testrol2 SUPERUSER;
diff --git a/src/test/modules/unsafe_tests/sql/alter_system_table.sql b/src/test/modules/unsafe_tests/sql/alter_system_table.sql
index c1515100845..1b0eb727eaa 100644
--- a/src/test/modules/unsafe_tests/sql/alter_system_table.sql
+++ b/src/test/modules/unsafe_tests/sql/alter_system_table.sql
@@ -64,6 +64,10 @@ ALTER TABLE pg_description SET SCHEMA public;
-- reserved tablespace name
CREATE TABLESPACE pg_foo LOCATION '/no/such/location';
+-- contains \n\r tablespace name
+CREATE TABLESPACE "invalid
+name" LOCATION '/no/such/location';
+
-- triggers
CREATE FUNCTION tf1() RETURNS trigger
LANGUAGE plpgsql
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..70b342abeb6 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -75,6 +75,8 @@ CREATE ROLE pg_abc; -- error
CREATE ROLE "pg_abc"; -- error
CREATE ROLE pg_abcdef; -- error
CREATE ROLE "pg_abcdef"; -- error
+CREATE ROLE "invalid
+rolename"; -- error
CREATE ROLE regress_testrol0 SUPERUSER LOGIN;
CREATE ROLE regress_testrolx SUPERUSER LOGIN;
diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out
index a90e39e5738..185880a3217 100644
--- a/src/test/regress/expected/tablespace.out
+++ b/src/test/regress/expected/tablespace.out
@@ -958,6 +958,11 @@ ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default
NOTICE: no matching relations in tablespace "regress_tblspace_renamed" found
ALTER MATERIALIZED VIEW ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default;
NOTICE: no matching relations in tablespace "regress_tblspace_renamed" found
+-- Should fail, contains \n in name
+ALTER TABLESPACE regress_tblspace_renamed RENAME TO "invalid
+name";
+ERROR: tablespace name "invalid
+name" contains a newline or carriage return character
-- Should succeed
DROP TABLESPACE regress_tblspace_renamed;
DROP SCHEMA testschema CASCADE;
diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql
index dfe3db096e2..c43a59e5957 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -430,6 +430,10 @@ ALTER MATERIALIZED VIEW ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPAC
ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default;
ALTER MATERIALIZED VIEW ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default;
+-- Should fail, contains \n in name
+ALTER TABLESPACE regress_tblspace_renamed RENAME TO "invalid
+name";
+
-- Should succeed
DROP TABLESPACE regress_tblspace_renamed;
