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@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.
>
> 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