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

On Thu, 27 Mar 2025 at 18:10, Andrew Dunstan <andrew(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)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(at)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