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 Sun, 6 Apr 2025 at 20:53, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2025-Apr-06, Andrew Dunstan wrote:
>
> > On 2025-03-28 Fr 10:43 AM, Nathan Bossart wrote:
>
> > > 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.
> >
> > Maybe. I don't think there is time for that for v18, so we'd have to
defer
> > this for now. I can live with that - it's been like this for a long
time.
>
> Grumble. I'd rather introduce a partial restriction now only for names
> that affect the tools failing outright (pg_dumpall in particular), than
> do nothing for this release. If we feel the need to extend the
> restriction later, that's easy to do and bothers nobody. We've wanted
> these characters to be forbidden on database names for a long time, but
> nobody has cared as much for their use on other types of names. I'm not
> even sure we'd support the idea of forbidding them on all names (though
> the SQL standard doesn't allow control chars in identifiers.)
>
> Another point is that we can easily have pg_upgrade check for invalid
> database and role names, but checking *all* names would be more onerous.
>
> I don't like the present implementation though, on translability
> grounds. I think the error message should appear at each callsite
> rather than be hardcoded in the new function, to avoid string building.
> I think it'd be cleaner if the new function (maybe "name_contains_crlf"
> or "is_identifier_awful") just returned a boolean based on strpbrk(),
> and the callsite throws the error.
>
> I wonder why does the patch restrict both database and role names. Does
> a user with a newline also cause pg_upgrade to fail? I mean, this
> thread started with a consideration for database names only, and the
> usage on role names seemed to have appeared out of nowhere in [1].

In my testing, pg_dumpall was not failing with roles/user (\n\r) in my
machine but due to the below comment in code, I restricted roles also.

+++ b/src/fe_utils/string_utils.c
> @@ -568,12 +568,6 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned
> char *str, size_t length,
> * Append the given string to the shell command being built in the buffer,
> * with shell-style quoting as needed to create exactly one argument.
> *
> - * Forbid LF or CR characters, which have scant practical use beyond
> designing
> - * security breaches. The Windows command shell is unusable as a conduit
> for
> - * arguments containing LF or CR characters. A future major release
> should
> - * reject those characters in CREATE ROLE and CREATE DATABASE, because use
> - * there eventually leads to errors here.
> - *
> * appendShellString() simply prints an error and dies if LF or CR
> appears.

> Cheers
>
> [1]
https://postgr.es/m/CAKYtNAoVKQL5rLD4P4hZXZSnThwO-j4q3Y1vTDHQGjzwC-kUJg@mail.gmail.com
>
> --
> Álvaro Herrera PostgreSQL Developer —
https://www.EnterpriseDB.com/
> "Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)

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