public inbox for [email protected]
help / color / mirror / Atom feedFrom: jian he <[email protected]>
To: Mahendra Singh Thalor <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Srinath Reddy <[email protected]>
Cc: [email protected]
Subject: Re: Non-text mode for pg_dumpall
Date: Mon, 10 Mar 2025 15:54:45 +0800
Message-ID: <CACJufxGYwhnaKk3X0uAjq4WbNym7k7axXAk95PhHNpxCV+cFAA@mail.gmail.com> (raw)
In-Reply-To: <CAKYtNAp+GqP66ORsihV+esFLG7hg9q6AvRNcJikELwVjezTW2Q@mail.gmail.com>
References: <CAKYtNAof2Rj0a7SZ3zCWQwNJSsciuPxQdJ4GAsX8GqDVKgf+9A@mail.gmail.com>
<[email protected]>
<CAKYtNAp+GqP66ORsihV+esFLG7hg9q6AvRNcJikELwVjezTW2Q@mail.gmail.com>
On Thu, Mar 6, 2025 at 12:49 AM Mahendra Singh Thalor
<[email protected]> wrote:
>
> Thanks Alvaro for feedback and review.
>
> On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <[email protected]> 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 <[email protected]> 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.
Attachments:
[application/octet-stream] v20-0001-pg_dumpall-deal-witth-newline-or-carriage-ret.no-cfbot (3.3K, 2-v20-0001-pg_dumpall-deal-witth-newline-or-carriage-ret.no-cfbot)
download
view thread (99+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Non-text mode for pg_dumpall
In-Reply-To: <CACJufxGYwhnaKk3X0uAjq4WbNym7k7axXAk95PhHNpxCV+cFAA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox