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 2025-04-02 We 7:45 AM, Mahendra Singh Thalor wrote:
> On Fri, 28 Mar 2025 at 20:13, Nathan Bossart
> <nathandbossart(at)gmail(dot)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(at)gmail(dot)com> wrote:
> >
> > ./psql postgres
> >
> > Hi,
> >
> > On Fri, Mar 28, 2025 at 5:08 PM Mahendra Singh Thalor
> <mahi6run(at)gmail(dot)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.

+void
+check_lfcr_in_objname(const char *objname, const char *objtype)
+{
+   /* Report error if name has \n or \r character. */
+   if (strpbrk(objname, "\n\r"))
+       ereport(ERROR,
+               (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+                errmsg("%s name contains a newline or carriage return
character", objtype),
+                errhint("a newline or a carriage return character is
not allowed in %s name\n"
+                    "here, given %s name is : \"%s\"", objtype,
objtype, objname));
+}

I don't think the errhint here adds much. If we're going to put the
offending name in an error message I think it possibly needs to be
escaped so it's more obvious where the CR/LF are. This should be in an
errdetail rather than an errhint.

+static void
+check_database_user_role_names_in_old_cluser(ClusterInfo *cluster)

s/cluser/cluster/

+   prep_status("Checking names of databases/users/roles ");

I would just say "Checking names of databases and roles".

+       pg_fatal("All the database, role/user names should have only
valid characters. A newline or \n"
+               "carriage return character is not allowed in these
object names.  To fix this, please \n"
+               "rename these names with valid names. \n"
+               "To see all %d invalid object names, refer
db_role_user_invalid_names.txt file. \n"
+               "    %s", count, output_path);

Also needs some cleanup.

cheers

andrew

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