Thread: Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
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.
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.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
>
>
>
> 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.
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
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,
>
> 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
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Srinath Reddy
Date:
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
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Andrew Dunstan
Date:
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
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Srinath Reddy
Date:
./psql postgres
Hi,
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
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Srinath Reddy
Date:
sorry for the noise ,previous response had my editor's formatting,just resending without that formatting.
./psql postgres
Hi,
./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
EDB: https://www.enterprisedb.com
postgres=#\q
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Andrew Dunstan
Date:
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
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Srinath Reddy
Date:
./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?
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?
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
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
databaseswith 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
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Andrew Dunstan
Date:
On 2025-03-27 Th 7:33 AM, Srinath Reddy wrote:
./psql postgresOn 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
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Andrew Dunstan
Date:
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
databaseswith 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
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
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.
>
>
> 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?
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Álvaro Herrera
Date:
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)
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
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.
>
> 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.
............
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'"
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.
*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'"
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)
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
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:
>
> 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 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.txt
Failure, 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
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Nathan Bossart
Date:
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
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Srinath Reddy
Date:
./psql postgres
Hi,
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
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.
May the force be with you,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
postgres=# \q
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Srinath Reddy
Date:
./psql postgres
postgres=# BEGIN;
Hi,
May the force be with you,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
postgres=# COMMIT;
postgres=# \q
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?
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.
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
postgres=# COMMIT;
postgres=# \q
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Mahendra Singh Thalor
Date:
On Fri, 28 Mar 2025 at 20:13, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.
Thanks Nathan for quick feedback.
>
> 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.
I think Andrew is planning this as a cleanup for "non-text pg_dumpall" patch. Andrew, please if you have some bandwidth, then please let us know your feedback for these patches.
>
> + /* 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. Fixed this and as Srinath pointed out, I moved it inside "src/backend/commands/define.c" file.
>
> 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.
>
As of now, we made this restriction to only "database/role/user" because dump is not allowed with these special characters.
yes, we can add these checks in grammar also. (probably we should add checks in grammar only). If others also feel same, I can try to add these checks in grammar.
On Sun, 30 Mar 2025 at 00:03, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> ./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.
>
Thanks Srinath for the review.
Ex: pg_upgrade --check:
--
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.
Thanks Nathan for quick feedback.
>
> 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.
I think Andrew is planning this as a cleanup for "non-text pg_dumpall" patch. Andrew, please if you have some bandwidth, then please let us know your feedback for these patches.
>
> + /* 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. Fixed this and as Srinath pointed out, I moved it inside "src/backend/commands/define.c" file.
>
> 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.
>
As of now, we made this restriction to only "database/role/user" because dump is not allowed with these special characters.
yes, we can add these checks in grammar also. (probably we should add checks in grammar only). If others also feel same, I can try to add these checks in grammar.
On Sun, 30 Mar 2025 at 00:03, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> ./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.
>
Thanks Srinath for the review.
In attached v05_0002* patch, instead of 2 exec commands, I merged into 1 and as per your comment, we will report all the invalid names at once and additionally we are giving count of invalid names.
Ex: pg_upgrade --check:
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking database connection settings ok
Checking names of databases/users/roles fatal
All the database, role/user names should have only valid characters. A newline or
carriage return character is not allowed in these object names. To fix this, please
rename these names with valid names.
To see all 5 invalid object names, refer db_role_user_invalid_names.txt file.
/home/mst/pg_all/head_pg/postgres/inst/bin/data/pg_upgrade_output.d/20250402T160610.664/db_role_user_invalid_names.txt
Failure, exiting
Here, I am attaching updated patches for review.
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachment
Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote
From
Andrew Dunstan
Date:
On 2025-04-02 We 7:45 AM, Mahendra Singh Thalor wrote:
On Fri, 28 Mar 2025 at 20:13, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.
Thanks Nathan for quick feedback.
>
> 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.
I think Andrew is planning this as a cleanup for "non-text pg_dumpall" patch. Andrew, please if you have some bandwidth, then please let us know your feedback for these patches.
Not sure if I will have time for this.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com