getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote

On Wed, Jan 29, 2025 at 9:55 PM Mahendra Singh Thalor <mahi6run@gmail.com>
wrote:

Hi,
While doing some testing with pg_dumpall, I noticed one weird behaviour.

While we create the database, we are allowing the database name with a new
line (if name is in double quote).
*For example*:

postgres=# create database "dbstr1;
dbstr 2";
CREATE DATABASE
postgres=#

Here, the database name is in 2 lines.

With the help of pg_dumpall, I tried to dump but I am getting an error for
the new line.

--

-- Database "dbstr1;
dbstr 2" dump
--

shell command argument contains a newline or carriage return: "
dbname='dbstr1;
dbstr 2'"

After this message, we are stopping the dump.

I have reproduced and verified the same.The reason is in runPgDump during
appendShellString for forming the pg_dump command , in
appendShellStringNoError we are considering the string as invalid if it has
'\n' and '\r'.

I think, if we are allowing new lines in the db name, then we should dump
it.

Agreed.

Regards,
Srinath Reddy Sadipiralla,
EDB: https://www.enterprisedb.com <http://www.enterprisedb.com/&gt;

On Thu, 30 Jan 2025 at 16:47, Srinath Reddy <srinath2133@gmail.com> wrote:

On Wed, Jan 29, 2025 at 9:55 PM Mahendra Singh Thalor <mahi6run@gmail.com>

wrote:

Hi,
While doing some testing with pg_dumpall, I noticed one weird behaviour.

While we create the database, we are allowing the database name with a

new line (if name is in double quote).

For example:

postgres=# create database "dbstr1;
dbstr 2";
CREATE DATABASE
postgres=#

Here, the database name is in 2 lines.

With the help of pg_dumpall, I tried to dump but I am getting an error

for the new line.

--
-- Database "dbstr1;
dbstr 2" dump
--

shell command argument contains a newline or carriage return: "

dbname='dbstr1;

dbstr 2'"

After this message, we are stopping the dump.

I have reproduced and verified the same.The reason is in runPgDump during

appendShellString for forming the pg_dump command , in
appendShellStringNoError we are considering the string as invalid if it has
'\n' and '\r'.

I think, if we are allowing new lines in the db name, then we should

dump it.

In another thread[1]names with \n\r in dbnames </messages/by-id/4ef51faa-993f-46ea-9e68-7baf736c07b8@dunslane.net&gt;, we have some discussions regarding \n\r in dbname and
they think that we should fix it.

As per code,

*
* 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.
*/
void
appendShellString(PQExpBuffer buf, const char *str)
{
if (!appendShellStringNoError(buf, str))
{
fprintf(stderr,
_("shell command argument contains a newline or carriage
return: \"%s\"\n"),
str);
exit(EXIT_FAILURE);
}
}

Here, we are mentioning that in future majar releases, we should reject
\n\r in *CREATE ROLE and CREATE DATABASE.*

Above comment was added in 2016.

commit 142c24c23447f212e642a0ffac9af878b93f490d
Author: Noah Misch <noah@leadboat.com>
Date: Mon Aug 8 10:07:46 2016 -0400

Reject, in pg_dumpall, names containing CR or LF.

These characters prematurely terminate Windows shell command
processing,
causing the shell to execute a prefix of the intended command. The
chief alternative to rejecting these characters was to bypass the
Windows shell with CreateProcess(), but the ability to use such names
has little value. Back-patch to 9.1 (all supported versions).

This change formally revokes support for these characters in database
names and roles names. Don't document this; the error message is
self-explanatory, and too few users would benefit. A future major
release may forbid creation of databases and roles so named. For now,
check only at known weak points in pg_dumpall. Future commits will,
without notice, reject affected names from other frontend programs.

Also extend the restriction to pg_dumpall --dbname=CONNSTR arguments
and
--file arguments. Unlike the effects on role name arguments and
database names, this does not reflect a broad policy change. A
migration to CreateProcess() could lift these two restrictions.

Reviewed by Peter Eisentraut.

Security: CVE-2016-5424

As per above comments, we can work on a patch which will reject \n\r in
roles and database names.

I will work on this.

[1]: names with \n\r in dbnames </messages/by-id/4ef51faa-993f-46ea-9e68-7baf736c07b8@dunslane.net&gt;
</messages/by-id/4ef51faa-993f-46ea-9e68-7baf736c07b8@dunslane.net&gt;

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Thu, 20 Mar 2025 at 23:39, Mahendra Singh Thalor <mahi6run@gmail.com>
wrote:

On Thu, 30 Jan 2025 at 16:47, Srinath Reddy <srinath2133@gmail.com> wrote:

On Wed, Jan 29, 2025 at 9:55 PM Mahendra Singh Thalor <

mahi6run@gmail.com> wrote:

Hi,
While doing some testing with pg_dumpall, I noticed one weird

behaviour.

While we create the database, we are allowing the database name with a

new line (if name is in double quote).

For example:

postgres=# create database "dbstr1;
dbstr 2";
CREATE DATABASE
postgres=#

Here, the database name is in 2 lines.

With the help of pg_dumpall, I tried to dump but I am getting an error

for the new line.

--
-- Database "dbstr1;
dbstr 2" dump
--

shell command argument contains a newline or carriage return: "

dbname='dbstr1;

dbstr 2'"

After this message, we are stopping the dump.

I have reproduced and verified the same.The reason is in runPgDump

during appendShellString for forming the pg_dump command , in
appendShellStringNoError we are considering the string as invalid if it has
'\n' and '\r'.

I think, if we are allowing new lines in the db name, then we should

dump it.

In another thread[1], we have some discussions regarding \n\r in dbname

and they think that we should fix it.

As per code,

*
* 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.
*/
void
appendShellString(PQExpBuffer buf, const char *str)
{
if (!appendShellStringNoError(buf, str))
{
fprintf(stderr,
_("shell command argument contains a newline or carriage

return: \"%s\"\n"),

str);
exit(EXIT_FAILURE);
}
}

Here, we are mentioning that in future majar releases, we should reject

\n\r in CREATE ROLE and CREATE DATABASE.

Above comment was added in 2016.

commit 142c24c23447f212e642a0ffac9af878b93f490d
Author: Noah Misch <noah@leadboat.com>
Date: Mon Aug 8 10:07:46 2016 -0400

Reject, in pg_dumpall, names containing CR or LF.

These characters prematurely terminate Windows shell command

processing,

causing the shell to execute a prefix of the intended command. The
chief alternative to rejecting these characters was to bypass the
Windows shell with CreateProcess(), but the ability to use such names
has little value. Back-patch to 9.1 (all supported versions).

This change formally revokes support for these characters in database
names and roles names. Don't document this; the error message is
self-explanatory, and too few users would benefit. A future major
release may forbid creation of databases and roles so named. For

now,

check only at known weak points in pg_dumpall. Future commits will,
without notice, reject affected names from other frontend programs.

Also extend the restriction to pg_dumpall --dbname=CONNSTR arguments

and

--file arguments. Unlike the effects on role name arguments and
database names, this does not reflect a broad policy change. A
migration to CreateProcess() could lift these two restrictions.

Reviewed by Peter Eisentraut.

Security: CVE-2016-5424

As per above comments, we can work on a patch which will reject \n\r in

roles and database names.

I will work on this.

[1] : names with \n\r in dbnames

--

Hi,
I tried to do some improvements for database names that have \n or \r in
dbname.

*Solution 1*:
As per code comments in appendShellString function, we can block database
creation with \n or \r.
sol1_v01* patch is doing the same for database creation.

*Solution 2:*
While dumping the database, report WARNING if the database name has \n or
\r and skip dump for a particular database but dump all other databases by
pg_dumpall.
sol2_v01* patch is doing this.

*Solution 3:*
While dumping the database, report FATAL if the database name has \n or \r
and add a hint message in FALAL (rename particular database to dump without
\n\r char).
sol3_v01* is doing the same.

Please review attached patches and let me know feedback.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Mon, Mar 24, 2025 at 11:21 PM Mahendra Singh Thalor <mahi6run@gmail.com>
wrote:

As per code,

*
* 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.
*/
void
appendShellString(PQExpBuffer buf, const char *str)
{
if (!appendShellStringNoError(buf, str))
{
fprintf(stderr,
_("shell command argument contains a newline or

carriage return: \"%s\"\n"),

str);
exit(EXIT_FAILURE);
}
}

Here, we are mentioning that in future majar releases, we should reject

\n\r in CREATE ROLE and CREATE DATABASE.

Above comment was added in 2016.

commit 142c24c23447f212e642a0ffac9af878b93f490d
Author: Noah Misch <noah@leadboat.com>
Date: Mon Aug 8 10:07:46 2016 -0400

Reject, in pg_dumpall, names containing CR or LF.

These characters prematurely terminate Windows shell command

processing,

causing the shell to execute a prefix of the intended command. The
chief alternative to rejecting these characters was to bypass the
Windows shell with CreateProcess(), but the ability to use such

names

has little value. Back-patch to 9.1 (all supported versions).

This change formally revokes support for these characters in

database

names and roles names. Don't document this; the error message is
self-explanatory, and too few users would benefit. A future major
release may forbid creation of databases and roles so named. For

now,

check only at known weak points in pg_dumpall. Future commits will,
without notice, reject affected names from other frontend programs.

Also extend the restriction to pg_dumpall --dbname=CONNSTR

arguments and

--file arguments. Unlike the effects on role name arguments and
database names, this does not reflect a broad policy change. A
migration to CreateProcess() could lift these two restrictions.

Reviewed by Peter Eisentraut.

Security: CVE-2016-5424

As per above comments, we can work on a patch which will reject \n\r in

roles and database names.

I will work on this.

[1] : names with \n\r in dbnames

--

Hi,
I tried to do some improvements for database names that have \n or \r in
dbname.

*Solution 1*:
As per code comments in appendShellString function, we can block database
creation with \n or \r.
sol1_v01* patch is doing the same for database creation.

*Solution 2:*
While dumping the database, report WARNING if the database name has \n or
\r and skip dump for a particular database but dump all other databases by
pg_dumpall.
sol2_v01* patch is doing this.

*Solution 3:*
While dumping the database, report FATAL if the database name has \n or \r
and add a hint message in FALAL (rename particular database to dump without
\n\r char).
sol3_v01* is doing the same.

Please review attached patches and let me know feedback.

Hi ,

I have reviewed all solutions but based on the commit message and comments,
it is clear that the goal is to *entirely forbid database names containing
carriage return (CR) or line feed (LF)* characters. *Solution 1 LGTM and
aligns with this approach* by enforcing the restriction at the time of
database creation, ensuring consistency throughout PostgreSQL. This
approach eliminates ambiguity and guarantees that such database names *cannot
be created or dumped with CR or LF.*

To validate this behavior, I have also implemented a *TAP test* for
Solution 1.
Thanks and regards,
Srinath Reddy Sadipiralla,
EnterpriseDB: http://www.enterprisedb.com

On 2025-03-26 We 3:04 AM, Srinath Reddy wrote:

I have reviewed all solutions but based on the commit message and
comments, it is clear that the goal is to *entirely forbid database
names containing carriage return (CR) or line feed (LF)* characters.
*Solution 1 LGTM and aligns with this approach* by enforcing the
restriction at the time of database creation, ensuring consistency
throughout PostgreSQL. This approach eliminates ambiguity and
guarantees that such database names *cannot be created or dumped with
CR or LF.*

To validate this behavior, I have also implemented a *TAP test* for
Solution 1.

You can still create a database with these using "CREATE DATABASE"
though. Shouldn't we should really be preventing that?

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:

You can still create a database with these using "CREATE DATABASE" though.
Shouldn't we should really be preventing that?

yes, solution 1 which i mentioned prevents these while we are using "CREATE
DATABASE".

/*
* Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
createdb_failure_params fparms;

+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database name contains a newline or carriage return character"),
+ errhint("newline or carriage return character is not allowed in database
name"));
+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR: database name contains a newline or carriage return character
HINT: newline or carriage return character is not allowed in database name

Thanks and regards,
Srinath Reddy Sadipiralla,
EnterpriseDB: http://www.enterprisedb.com

postgres=#\q

sorry for the noise ,previous response had my editor's formatting,just
resending without that formatting.

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:

You can still create a database with these using "CREATE DATABASE" though.
Shouldn't we should really be preventing that?

yes, solution 1 which I mentioned prevents these while we are using "CREATE
DATABASE".

/*
* Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
createdb_failure_params fparms;

+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database name contains a newline or carriage return character"),
+ errhint("newline or carriage return character is not allowed in database
name"));
+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR: database name contains a newline or carriage return character
HINT: newline or carriage return character is not allowed in database name

Thanks and regards,
Srinath Reddy Sadipiralla,
EDB: https://www.enterprisedb.com

postgres=#\q

On 2025-03-26 We 8:52 AM, Srinath Reddy wrote:

sorry for the noise ,previous response had my editor's formatting,just
resending without that formatting.

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net>
wrote:

You can still create a database with these using "CREATE DATABASE"
though. Shouldn't we should really be preventing that?

yes, solution 1 which I mentioned prevents these while we are
using "CREATE DATABASE".

/*
  * Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt
*stmt)
  CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
  createdb_failure_params fparms;

+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database name contains a newline or carriage return character"),
+ errhint("newline or carriage return character is not allowed in 
database name"));
+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR:  database name contains a newline or carriage return character
HINT:  newline or carriage return character is not allowed in database
name

Yes, sorry, I misread the thread. I think we should proceed with options
1 and 3 i.e. prevent creation of new databases with a CR or LF, and have
pgdumpall exit with a more useful error message.

Your invention of an is_name_contain_lfcr() function is unnecessary - we
can just use the standard library function strpbrk() to look for a CR or LF.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

./psql postgres

On Thu, Mar 27, 2025 at 4:16 PM Andrew Dunstan <andrew@dunslane.net> wrote:

Yes, sorry, I misread the thread. I think we should proceed with options 1
and 3 i.e. prevent creation of new databases with a CR or LF, and have
pgdumpall exit with a more useful error message.

agreed.

Your invention of an is_name_contain_lfcr() function is unnecessary - we
can just use the standard library function strpbrk() to look for a CR or LF.

makes sense,but I have a dumb doubt why in appendShellStringNoError() it
still continues even after it found CR or LF? ,AFAIK The reasoning is this
function is designed to silently filter out \n and \r while still producing
a usable shell-safe argument. It informs the caller of the issue (false
return value) but does not abruptly stop execution,then the caller will
decide what to do but every place this function is called they are just
throwing the error.

if we could just break the loop right after we found \n or \r in
appendShellStringNoError() we can also use strpbrk() here and during
creation of new database as you suggested.

thoughts?

May the force be with you,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

postgres=#\q

On Thu, 27 Mar 2025 at 16:16, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2025-03-26 We 8:52 AM, Srinath Reddy wrote:

sorry for the noise ,previous response had my editor's formatting,just resending without that formatting.

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:

You can still create a database with these using "CREATE DATABASE" though. Shouldn't we should really be preventing that?

yes, solution 1 which I mentioned prevents these while we are using "CREATE DATABASE".

/*
* Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
createdb_failure_params fparms;

+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database name contains a newline or carriage return character"),
+ errhint("newline or carriage return character is not allowed in database name"));
+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR: database name contains a newline or carriage return character
HINT: newline or carriage return character is not allowed in database name

Yes, sorry, I misread the thread. I think we should proceed with options 1 and 3 i.e. prevent creation of new databases with a CR or LF, and have pgdumpall exit with a more useful error message.

Your invention of an is_name_contain_lfcr() function is unnecessary - we can just use the standard library function strpbrk() to look for a CR or LF.

cheers

Thanks Andrew and Srinath for feedback.

Yes, we should use the strpbrk function. Fixed.

Here, I am attaching an updated patch which has check in createdb and
RenameDatabase. For older versions, we can add more useful error
message (like: rename database as database has \n\r")

I will add some TAP tests and will make patches for older branches.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On 2025-03-27 Th 7:33 AM, Srinath Reddy wrote:

./psql postgres

On Thu, Mar 27, 2025 at 4:16 PM Andrew Dunstan <andrew@dunslane.net>
wrote:

Yes, sorry, I misread the thread. I think we should proceed with
options 1 and 3 i.e. prevent creation of new databases with a CR
or LF, and have pgdumpall exit with a more useful error message.

agreed.

Your invention of an is_name_contain_lfcr() function is
unnecessary - we can just use the standard library function
strpbrk() to look for a CR or LF.

makes sense,but I have a dumb doubt why in appendShellStringNoError()
it still continues even after it found CR or LF? ,AFAIK The reasoning
is this function is designed to silently filter out \n and \r while
still producing a usable shell-safe argument. It informs the caller of
the issue (false return value) but does not abruptly stop
execution,then the caller will decide what to do but every place this
function is called they are just throwing the error.

if we could just break the loop right after we found \n or \r in
appendShellStringNoError() we can also use strpbrk() here and during
creation of new database as you suggested.

thoughts?

I don't know. If you want to submit a patch cleaning it up go ahead. But
right now I just want to get this original issue cleaned up to go along
with the pg_dumpall improvements.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On 2025-03-27 Th 7:57 AM, Mahendra Singh Thalor wrote:

On Thu, 27 Mar 2025 at 16:16, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2025-03-26 We 8:52 AM, Srinath Reddy wrote:

sorry for the noise ,previous response had my editor's formatting,just resending without that formatting.

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:

You can still create a database with these using "CREATE DATABASE" though. Shouldn't we should really be preventing that?

yes, solution 1 which I mentioned prevents these while we are using "CREATE DATABASE".

/*
* Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
createdb_failure_params fparms;

+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database name contains a newline or carriage return character"),
+ errhint("newline or carriage return character is not allowed in database name"));
+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR: database name contains a newline or carriage return character
HINT: newline or carriage return character is not allowed in database name

Yes, sorry, I misread the thread. I think we should proceed with options 1 and 3 i.e. prevent creation of new databases with a CR or LF, and have pgdumpall exit with a more useful error message.

Your invention of an is_name_contain_lfcr() function is unnecessary - we can just use the standard library function strpbrk() to look for a CR or LF.

cheers

Thanks Andrew and Srinath for feedback.

Yes, we should use the strpbrk function. Fixed.

Here, I am attaching an updated patch which has check in createdb and
RenameDatabase. For older versions, we can add more useful error
message (like: rename database as database has \n\r")

I will add some TAP tests and will make patches for older branches.

I don't think we can backpatch this. It's a behaviour change.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 27 Mar 2025 at 18:10, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2025-03-27 Th 7:57 AM, Mahendra Singh Thalor wrote:

On Thu, 27 Mar 2025 at 16:16, Andrew Dunstan <andrew@dunslane.net>

wrote:

On 2025-03-26 We 8:52 AM, Srinath Reddy wrote:

sorry for the noise ,previous response had my editor's formatting,just

resending without that formatting.

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net>

wrote:

You can still create a database with these using "CREATE DATABASE"

though. Shouldn't we should really be preventing that?

yes, solution 1 which I mentioned prevents these while we are using

"CREATE DATABASE".

/*
* Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt

*stmt)

CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
createdb_failure_params fparms;

+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("database name contains a newline or carriage return

character"),

+ errhint("newline or carriage return character is not allowed in

database name"));

+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR: database name contains a newline or carriage return character
HINT: newline or carriage return character is not allowed in database

name

Yes, sorry, I misread the thread. I think we should proceed with

options 1 and 3 i.e. prevent creation of new databases with a CR or LF, and
have pgdumpall exit with a more useful error message.

Your invention of an is_name_contain_lfcr() function is unnecessary -

we can just use the standard library function strpbrk() to look for a CR or
LF.

cheers

Thanks Andrew and Srinath for feedback.

Yes, we should use the strpbrk function. Fixed.

Here, I am attaching an updated patch which has check in createdb and
RenameDatabase. For older versions, we can add more useful error
message (like: rename database as database has \n\r")

I will add some TAP tests and will make patches for older branches.

I don't think we can backpatch this. It's a behaviour change.

We can add a good error message in pg_dumpall and pg_dump for all older
branches by checking dbname.

[mst@localhost postgres]$ git diff
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 2ea574b0f06..991dd4db2ca 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1611,6 +1611,10 @@ dumpDatabases(PGconn *conn)
if (strcmp(dbname, "template0") == 0)
continue;

+               /* Report fatal if database name have \n\r */
+               if (strpbrk(dbname, "\n\r"))
+                       pg_fatal("database name has newline or carriage
return so stoping dump. To fix, rename dbname.");
+
/* Skip any explicitly excluded database */
if (simple_string_list_member(&database_exclude_names,
dbname))
{
[mst@localhost postgres]$

This will change the error message in older versions. If it is OK to report
more readable errors in back branches, then we can do the above change.

Thoughts?

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On 2025-Mar-27, Andrew Dunstan wrote:

I don't think we can backpatch this. It's a behaviour change.

I agree, we can't.

Also, if we're going to enforce this rule, then pg_upgrade --check needs
to alert users that they have a database name that's no longer valid.
That needs to be part of this patch as well. We should also ensure that
these are the only problem characters, which IMO means it should add a
test for pg_dumpall that creates a database whose name has all possible
characters and ensures that it is dumpable.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

On Thu, 27 Mar 2025 at 18:33, Álvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2025-Mar-27, Andrew Dunstan wrote:

I don't think we can backpatch this. It's a behaviour change.

I agree, we can't.

Also, if we're going to enforce this rule, then pg_upgrade --check needs
to alert users that they have a database name that's no longer valid.
That needs to be part of this patch as well. We should also ensure that
these are the only problem characters, which IMO means it should add a
test for pg_dumpall that creates a database whose name has all possible
characters and ensures that it is dumpable.

--
Álvaro Herrera Breisgau, Deutschland —

https://www.EnterpriseDB.com/

"La virtud es el justo medio entre dos defectos" (Aristóteles)

Thanks Álvaro.

Yes, I also agree with you. "pg_upgrade --check" should alert users
regarding database names because pg_upgrade is failing if we have \n\r in
dbname(old cluster).

*pg_upgrade --check: This is passing but **pg_upgrade is failing.*
............
Checking for new cluster tablespace directories ok
*Clusters are compatible*

*pg_upgrade*:
.........................
Creating dump of global objects ok
Creating dump of database schemas

*shell command argument contains a newline or carriage return:
"dbname='invaliddb'"*

As a part of this patch, we can teach *pg_upgrade* to alert users regarding
database names with invalid characters or we can keep old behavior as
upgrade is already failing without this patch also.

I will try to make a separate patch to teach "*pg_upgrade --check"* to
alert users regarding database/user/role with \n\r in the old cluster.

Here, I am attaching an updated patch for review.

This patch has changes for: CREATE DATABASE, CREATE ROLE, CREATE USER and
RENAME DATABASE/USER/ROLE and have some tests also. (EXCEPT RENAME test
case)

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Fri, 28 Mar 2025 at 01:37, Mahendra Singh Thalor <mahi6run@gmail.com>
wrote:

On Thu, 27 Mar 2025 at 18:33, Álvaro Herrera <alvherre@alvh.no-ip.org>

wrote:

On 2025-Mar-27, Andrew Dunstan wrote:

I don't think we can backpatch this. It's a behaviour change.

I agree, we can't.

Also, if we're going to enforce this rule, then pg_upgrade --check needs
to alert users that they have a database name that's no longer valid.
That needs to be part of this patch as well. We should also ensure that
these are the only problem characters, which IMO means it should add a
test for pg_dumpall that creates a database whose name has all possible
characters and ensures that it is dumpable.

--
Álvaro Herrera Breisgau, Deutschland —

https://www.EnterpriseDB.com/

"La virtud es el justo medio entre dos defectos" (Aristóteles)

Thanks Álvaro.

Yes, I also agree with you. "pg_upgrade --check" should alert users

regarding database names because pg_upgrade is failing if we have \n\r in
dbname(old cluster).

pg_upgrade --check: This is passing but pg_upgrade is failing.
............
Checking for new cluster tablespace directories ok
*Clusters are compatible*

pg_upgrade:
.........................
Creating dump of global objects ok
Creating dump of database schemas
shell command argument contains a newline or carriage return:

"dbname='invalid

db'"

In the attached patch, instead of the above error, we will get proper ALERT
for invalid names even with "pg_upgrade --check" also.

*Ex:*

Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking database connection settings ok
Checking names of databases/users/roles fatal

*All the database names should have only valid characters. A newline or
carriage return character is not allowed in database name. To fix this,
please rename database names with valid names.
/home/mst/pg_all/head_pg/postgres/inst/bin/data/pg_upgrade_output.d/20250328T164926.680/db_role_invalid_names.txtFailure,
exiting*

As a part of this patch, we can teach pg_upgrade to alert users regarding

database names with invalid characters or we can keep old behavior as
upgrade is already failing without this patch also.

I will try to make a separate patch to teach "pg_upgrade --check" to

alert users regarding database/user/role with \n\r in the old cluster.

Here, I am attaching an updated patch for review.

This patch has changes for: CREATE DATABASE, CREATE ROLE, CREATE USER and

RENAME DATABASE/USER/ROLE and have some tests also. (EXCEPT RENAME test
case)

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

Here, I am attaching updated patches for review.

v04_001* has the changes for CREATE DATABASE/ROLE/USER and
v04_002* has the changes into pg_upgrade to give ALERTS for invalid names.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Fri, Mar 28, 2025 at 05:08:26PM +0530, Mahendra Singh Thalor wrote:

Here, I am attaching updated patches for review.

v04_001* has the changes for CREATE DATABASE/ROLE/USER and
v04_002* has the changes into pg_upgrade to give ALERTS for invalid names.

In general, +1 for these changes. Thanks for picking this up.

If these are intended for v18, we probably should have a committer
attached to it soon. I'm not confident that I'll have time for it,
unfortunately.

+	/* Report error if dbname have newline or carriage return in name. */
+	if (strpbrk(dbname, "\n\r"))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+				errmsg("database name contains a newline or carriage return character"),
+				errhint("newline or carriage return character is not allowed in database name"));

I think it would be better to move this to a helper function instead of
duplicating this code in several places.

Taking a step back, are we sure that 1) this is the right place to do these
checks and 2) we shouldn't apply the same restrictions to all names? I'm
wondering if it would be better to add these checks to the grammar instead
of trying to patch up all the various places they are used in the tree.

--
nathan

./psql postgres

Hi,

On Fri, Mar 28, 2025 at 5:08 PM Mahendra Singh Thalor <mahi6run@gmail.com>
wrote:

v04_001* has the changes for CREATE DATABASE/ROLE/USER and
v04_002* has the changes into pg_upgrade to give ALERTS for invalid names.

i have reviewed these patches,in v04_002* i think it would be better if log
all the names like database ,roles/users names then throw error instead of
just logging database names into db_role_invalid_names and throwing
error,which helps the user to see at once all the invalid names and resolve
then again try pg_upgrade,so here i am attaching delta patch for the same
,except this the patches LGTM.

May the force be with you,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

postgres=# \q

./psql postgres
postgres=# BEGIN;

Hi,

On Fri, Mar 28, 2025 at 8:13 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

+       /* Report error if dbname have newline or carriage return in name.
*/
+       if (strpbrk(dbname, "\n\r"))
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+                               errmsg("database name contains a newline
or carriage return character"),
+                               errhint("newline or carriage return
character is not allowed in database name"));

I think it would be better to move this to a helper function instead of
duplicating this code in several places.

agreed,we can do something like this

static void
validate_name(const char *name, const char *object_type)
{
if (strpbrk(name, "\n\r"))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
errmsg("%s name contains a newline or carriage return
character", object_type),
errhint("Newline or carriage return character is not
allowed in %s name", object_type));
}

where object_type is database or role/user name
,is src/backend/commands/define.c best to define this function?

Taking a step back, are we sure that 1) this is the right place to do these
checks and 2) we shouldn't apply the same restrictions to all names? I'm
wondering if it would be better to add these checks to the grammar instead
of trying to patch up all the various places they are used in the tree.

hmm... need to think.

May the force be with you,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

postgres=# COMMIT;
postgres=# \q