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

Thanks Andrew for the review.

On Sat, 5 Apr 2025 at 20:12, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> 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/usersnames then throw error instead of just logging database names into db_role_invalid_names and throwing
error,whichhelps the user to see at once all the invalid names and resolve then again try pg_upgrade,so here i am
attachingdelta 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
theinvalid 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
possiblyneeds to be escaped so it's more obvious where the CR/LF are. This should be in an errdetail rather than an
errhint.

Fixed. I replaced errhint with errdetail as "errdetail("invalid %s
name is  \"%s\"", objtype, objname));"

>
> +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".

Fixed.

>
>
> +       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

Here, I am attaching updated patches for review.

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