Обсуждение: Quoting issues with createdb

Hi all,

createdb has a couple of issues with its quoting.  For example take
that, which can be confusing:
$ createdb --lc-ctype="en_US.UTF-8';create table aa();select '1" popo
createdb: error: database creation failed: ERROR:  CREATE DATABASE
cannot run inside a transaction block

The root of the issue is that any values added by the command caller
with --lc-collate, --lc-ctype or --encoding are not quoted properly,
and in all three cases it means that the quoting needs to be
encoding-sensitive (Tom mentioned me directly that part).  This proper
quoting can be achieved using appendStringLiteralConn() from
string_utils.c, at the condition of taking the connection to the
server before building the CREATE DATABASE query.

Note that for --encoding, this is less of a problem as there is some
extra validation with pg_char_to_encoding(), but it seems better to me
to be consistent.

So this gives the patch attached, where the error becomes:
ERROR:  invalid locale name: "en_US.UTF-8';create table aa();select '1"

Any opinions?
--
Michael

> On 14 Feb 2020, at 05:10, Michael Paquier <michael@paquier.xyz> wrote:

> createdb has a couple of issues with its quoting.  For example take
> that, which can be confusing:
> $ createdb --lc-ctype="en_US.UTF-8';create table aa();select '1" popo
> createdb: error: database creation failed: ERROR:  CREATE DATABASE
> cannot run inside a transaction block

Nice catch!

> The root of the issue is that any values added by the command caller
> with --lc-collate, --lc-ctype or --encoding are not quoted properly,
> and in all three cases it means that the quoting needs to be
> encoding-sensitive (Tom mentioned me directly that part).  This proper
> quoting can be achieved using appendStringLiteralConn() from
> string_utils.c, at the condition of taking the connection to the
> server before building the CREATE DATABASE query.

Makes sense, it aligns it with other utils and passes all the tests.  +1 on the
fix.

> Any opinions?

I would've liked a negative test basically along the lines of your example
above.  If we left a hole the size of this, it would be nice to catch it from
accidentally happening again.

diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index c0f6067a92..afd128deba 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -3,7 +3,7 @@ use warnings;

 use PostgresNode;
 use TestLib;
-use Test::More tests => 13;
+use Test::More tests => 14;

 program_help_ok('createdb');
 program_version_ok('createdb');
@@ -24,3 +24,6 @@ $node->issues_sql_like(

 $node->command_fails([ 'createdb', 'foobar1' ],
    'fails if database already exists');
+
+$node->command_fails(['createdb', '-l', 'C\';SELECT 1;' ],
+   'fails on incorrect locale');

cheers ./daniel

On Thu, Feb 27, 2020 at 12:00:11AM +0100, Daniel Gustafsson wrote:
> Makes sense, it aligns it with other utils and passes all the tests.  +1 on the
> fix.

Thanks for the review.

> I would've liked a negative test basically along the lines of your example
> above.  If we left a hole the size of this, it would be nice to catch it from
> accidentally happening again.

No arguments against that.

>  program_help_ok('createdb');
>  program_version_ok('createdb');
> @@ -24,3 +24,6 @@ $node->issues_sql_like(
>
>  $node->command_fails([ 'createdb', 'foobar1' ],
>     'fails if database already exists');
> +
> +$node->command_fails(['createdb', '-l', 'C\';SELECT 1;' ],
> +   'fails on incorrect locale');

One problem with this way of testing things is that you don't check
the exact error message triggered, and this command fails with or
without the patch, so you don't actually know if things are correctly
patched up or not.  What should be used instead is command_checks_all,
available down to 11 where we check for a failure and a match with the
error string generated.  I have used that, and applied the patch down
to 9.5.
--
Michael