Re: Non-text mode for pg_dumpall

On Thu, Mar 6, 2025 at 12:49 AM Mahendra Singh Thalor
<mahi6run(at)gmail(dot)com> wrote:
>
> Thanks Alvaro for feedback and review.
>
> On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > Disclaimer: I didn't review these patches fully.
> >
> > On 2025-Mar-05, Mahendra Singh Thalor wrote:
> >
> > > On Wed, 5 Mar 2025 at 01:02, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > >
> > > > A database name containing a newline breaks things for this patch:
> > > >
> > > > CREATE DATABASE "foo
> > > > bar";
> >
> > > I also reported this issue on 29-01-2025. This breaks even without this
> > > patch also.
> >
> > Okay, we should probably fix that, but I think the new map.dat file your
> > patch adds is going to make the problem worse, because it doesn't look
> > like you handled that case in any particular way that would make it not
> > fail. I think it would be good to avoid digging us up even deeper in
> > that hole. More generally, the pg_upgrade tests contain some code to
> > try database names with almost all possible ascii characters (see
> > generate_db in pg_upgrade/t/002_pg_upgrade.pl); it would be good to
> > ensure that this new functionality also works correctly for that --
> > perhaps add an equivalent test to the pg_dumpall test suite.
>
> In the attached patch, I tried to solve the problem of the map.dat
> file. I will do more analysis based on dbnames in 002_pg_upgrade.pl
> file.
>

hi.

/*
* 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.
* appendShellStringNoError() omits those characters from the result, and
* returns false if there were any.
*/
void
appendShellString(PQExpBuffer buf, const char *str)

per above comments,
we need to disallow LF/CR in database name and role name when issuing
shell command.

rolename LF/CR issue already being handled in
src/bin/pg_dump/pg_dumpall.c: while(getopt_long) code:
case 3:
use_role = pg_strdup(optarg);
appendPQExpBufferStr(pgdumpopts, " --role ");
appendShellString(pgdumpopts, use_role);

we can fail earlier also for database names in dumpDatabases, right
after executeQuery.
Please check attached, which is based on *v20*.

in V21, src/bin/pg_dump/pg_dumpall.c:
+#include "common_dumpall_restore.h"
happened within v21-0001 and v21-0002, it is being included twice.